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

Simplify #include statements #322

Merged
merged 5 commits into from
Jun 3, 2019

Conversation

manpen
Copy link
Contributor

@manpen manpen commented May 27, 2019

Commit 4736fde moved header files into include/networkit which resulted in error-prone #include statements such as

#include "../../../../include/networkit/io/METISGraphReader.hpp"

They should be written as:

#include <networkit/io/METISGraphReader.hpp>

The first option is harder to read, less flexible and tedious to write (as I've seen while working on #320).
Also the second format is more consistent for external users linking against NetworKit.

This PR does the following:

  • Add search path in CMakeList.txt with higher priority than system search path (to prevent usage of installed headers)
  • Rewrite include in networkit/cpp/ and include/networkit such that no relative includes are used (see below)
  • Rewrote some occurences of #include "omp.h" to #include <omp.h>

If you think this change is worth-while, there's a decision to make:
How do we treat includes of siblings within the same directory (e.g. A/bar.hpp includes A/foo.hpp). Do you prefer #include "foo.hpp" (shorter) or #include <networkit/A/foo.hpp> (more verbose).

I feel the more consistent second options is better, as it leaves no exceptions which have to communicated in the style guide.

@manpen manpen changed the title Feature/simpler includes Simplify #include statements May 27, 2019
@manpen
Copy link
Contributor Author

manpen commented May 27, 2019

Just to support my claim on: there are a lot of includes in subdirectories -- typically tests -- which in fact use the wrong number of "../". One of many examples is networkit/cpp/algebraic/test/GraphBLASGTest.cpp:

#include "../../../include/networkit/algebraic/GraphBLAS.hpp"

This points to [PRJROOT]/networkit/include/networkit/algebraic/GraphBLAS.hpp rather than [PRJROOT]/include/networkit/algebraic/GraphBLAS.hpp.

@avdgrinten
Copy link
Contributor

Thank you for the PR, Manuel. This was already on our TODO list since we moved the headers to include/. Hence, I am very grateful that you did the work to implement this :).

The second option for local files should be preferred in public headers. In principle, the first option could be used for private headers but as far as I can see, we don't have private headers right now.

@manpen
Copy link
Contributor Author

manpen commented May 28, 2019

I consider this PR complete. We should definitely add some automatic checks for CI. But this should be done in a separate PR. Please let me know, if you spot anything else.

@avdgrinten
Copy link
Contributor

What do you mean by automated checks for CI? Checking that nobody introduces relative includes again?

@manpen
Copy link
Contributor Author

manpen commented May 28, 2019

Exactly. I used a script for these conversions which could in theory be modified. However, I think it's for the best, if we first commit to some form of infrastructure for that; otherwise each checker will solve similar issues such as finding all relevant header/source files redundantly.

@avdgrinten
Copy link
Contributor

This has been open for a week now; I don't see any issues with this patch set, so I will merge it. Future PRs should now always use <> includes.

@avdgrinten avdgrinten merged commit dc92206 into networkit:Dev Jun 3, 2019
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.

None yet

2 participants