Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include-what-you-use changes and #include section reorganization #452

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nealsid
Copy link
Contributor

@nealsid nealsid commented Sep 29, 2022

This is an initial run of IWYU over PCM sources, but isn't ready for review - just checking builds on platforms I don't have installed (FreeBSD, Windows)

@nealsid nealsid changed the title Not ready for review - IWYU changes Include-what-you-use changes and #include section reorganization Sep 30, 2022
@nealsid
Copy link
Contributor Author

nealsid commented Sep 30, 2022

This is ready for review now if someone is able to take a look. I'm not sure why the macOS build is failing, but it seems unrelated and builds fine on my machine. Thank you!

This commit applies changes suggested by include-what-you-use.  I did
not apply all of them as there are some false positives/negatives and
it did not recognize forward decls in some cases, and also tends to
suggest headers that should not be included, such as files in the
bits/ directory on Linux, but most changes have been applied.
This commit applies a few cleanups to the #include sections:

- Remove old version checks for Visual Studio, SUSE Linux, and DragonFly BSD
- Alphabetize the includes
- Consolidate multiple MSC_VER checks in the same file
Copy link
Contributor

@rdementi rdementi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! Please see my comments

Comment on lines +22 to 24
}

namespace pcm {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these lines can be deleted

Comment on lines +25 to +26
#include <vector>
#include <vector>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2x include vector

#include <iomanip>
#include <string>
#include <utility>
#include <utility>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2x include utility

#include <sys/time.h> // for gettimeofday()
#endif
#include "types.h"
#include "types.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2x include types.h

#include <fstream>
#include <iomanip>
#include <iostream>
#include <iostream>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2x include iostream

Comment on lines +21 to +24
}

namespace pcm
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

drop these lines "} namespace pcm {"

class HyperThread;
class ServerUncore;
class ClientUncore;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

drop these lines

#else
#include <intrin.h>
#endif

namespace pcm {
class PCM;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

drop these lines "} namespace pcm {"


namespace pcm {
class MMIORange;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

drop these lines "} namespace pcm {"

class PCM;
class CoreTaskQueue;
class SystemRoot;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

drop these lines "} namespace pcm {"

@nealsid
Copy link
Contributor Author

nealsid commented Oct 5, 2022

Thank you for accepting this contribution - it is always a team effort by multiple people. Unfortunately I have some travel this week so my time to update the change will limited and I will better know my schedule next week.

Again, many thanks for accepting this.

@nealsid
Copy link
Contributor Author

nealsid commented Oct 5, 2022

Thank you for accepting this contribution - it is always a team effort by multiple people. Unfortunately I have some travel this week so my time to update the change will limited and I will better know my schedule next week.

Again, many thanks for accepting this.

Also, I meant to address if anyone thought running IWYU as a regular process should be considered. In my experience it has a few false positives (e.g., the reason forward decls are in a separate namespace block is that IWYU doesn't detect them if they are not) that would make it not worthwhile. It's probably better just to run it at regular intervals rather than keep it president in the automation. Thanks!

@nealsid
Copy link
Contributor Author

nealsid commented Mar 22, 2023

Hey there, I unfortunately got a little sidetracked for a few months with moving and some non-project related tasks, but could definitely still update this and resubmit it. Need to get my VMs set up again so it may be a couple of weeks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants