Project

General

Profile

Task #1557

Uniform usage of include paths

Added by Roland Schulz over 5 years ago. Updated almost 5 years ago.

Status:
Closed
Priority:
Normal
Assignee:
-
Category:
core library
Target version:
Difficulty:
uncategorized
Close

Description

See beginning of the discussion here: https://gerrit.gromacs.org/#/c/3531/

Adding some more information here, because the commit message is getting too long.

I'm aware of 3 proposals:
  1. the one in Gerrit by me (see above) which is the same as Google Style Guidelines
  2. Mark's: use relative paths for other files in the same module
  3. Teemu's: use relative paths for public headers
Advantages of 1 over 2:
  • C++ files can be moved without changing includes. Headers can be moved with simple automatic change (e.g. sed)
  • Easier to auto-generate include-paths (e.g. IWYU, generators)
Advantages of 2 over 1:
  • shorter path for headers local to current directory
Advantages of 1 over 3:
  • reduced accidental complexity ( e.g no need to remember different rules for public/private headers)
  • possible to auto-generate include paths without dependency on CMakeLists.txt (to check for public/private)
  • C++ files can be moved without changing includes. Headers can be moved with simple automatic change (e.g. sed)
Advantages of 3 over 1:
  • Possible to use headers installed into somethings else than .../gromacs
  • sometimes shorter path for headers local to current directory
  • Paths show difference between public/private headers
Advantages of 2 over 3:
  • reduced accidental complexity ( e.g no need to remember different rules for public/private headers)
  • possible to auto-generate include paths without dependency on CMakeLists.txt (to check for public/private)
  • sometimes shorter path for headers local to current directory
Advantages of 3 over 2:
  • Possible to use headers installed into somethings else than .../gromacs
  • Paths show difference between public/private headers

Associated revisions

Revision 20191202 (diff)
Added by Roland Schulz over 5 years ago

Use full path for legacyheaders

Advantages:
simpler to setup tooling / IDE (just one path)
show immediately all usage of legacyheaders
No risk of accidentally working includes (together with using relative paths)

Used:
find src/gromacs/ src/programs/ src/testutils/ -name '*.c' -o -name '*.cpp' \
-o -name '*.h' -o -name '*.cu' -o -name '*.cuh' -o -name '*.pre'|
xargs -n1 python makeabspath.py

Manual changes to cmake and doxygen files to remove legacyheader include path.

makeabspath.py:
https://gist.github.com/rolandschulz/ee636f517d75e48bc68c

Related to #1557.

Change-Id: I6c1b51a573acfe7b0123485405f2494e97715f53

Revision 1bf821d4 (diff)
Added by Teemu Murtola about 5 years ago

Include directive sorter

To facilitate discussion on how we want to sort the includes, the tool
supports a few different styles (with a --style option) that at least
roughly correspond to what has been proposed in #1557 or related
discussions. Can be removed later when the final style is agreed on.

Having an automatic sorter also allows relatively easily using other
tools that may sort the includes as part of their operation (the
automatic sorter can just be run afterwards).

Made gmxtree.py aware of the location of the list of installer headers,
removing the need to pass the file on the command line. This simplifies
manual invocation (in particular for the include sorter).

It only sorts a list of consecutive #include lines, possibly separated
by empty lines. Comments between the lines or #ifdefs are left alone,
and only includes on both sides sorted independently. This is by
design, as it keeps the script relatively simple, and also allows using
comments or similar to override the sorting for special cases. Some
manual formatting may be required to make files with heavy use of
conditinal includes to look nice (to move headers to the appropriate
side of the #ifdefs), but after that, the tool should work quite well.

Add a git attribute to allow easily applying the script in combination
with a file list produced by admin/reformat_all.sh, and extend
reformat_all.sh such that it can be be run as
admin/reformat_all.sh includesort -B=<builddir>
to sort all files with the attribute.

Related to #1557.

Change-Id: I362d7c5131a2e6952bbededfd51da4c4e1fc4a7e

History

#1 Updated by Teemu Murtola over 5 years ago

  • Category set to core library
  • Target version set to 5.x
I think you still misunderstood me. ;) What I was proposing that you split the advantages/disadvantages over these changes:
  • Remove legacyheaders/ from the include path (so that ~everything is included as gromacs/legacyheaders/).
    • I see no disadvantages here, and your "simpler to setup tooling" advantage mostly applies here. The only short-term disadvantage is that it may obscure where some manual curation of the #includes has already taken place, but that should be possible to see from other stuff as well (like whether stuff is even approximately sorted). It also has an additional advantage that one can use simple grep to find all occurrences of intermodule dependencies.
  • Remove relative paths from public headers.
    • Some of the disadvantages only apply here, and I suspect that this is currently the source of most accidental complexity.
  • Remove relative module-local paths.
    • This is the only part where there have been multiple proposals, so the comparison of advantages/disadvantages should also focus here.
  • Remove all other misc. relative paths (e.g., "../" paths within modules).
    • I think this has the largest advantages for, e.g., sed or other automatic scripting. Even if a header can be included as "gromacs/foo/bar.h" or "bar.h", but nothing else, the sed script will only be slightly longer.

As a general comment, code is written for humans to read, not for computers to generate. ;) It is of course nice if we can achieve both at the same time, but more easily understandable code is in my mind worth a bit of additional scripting. I can write an include sorter for any of the proposals (it shouldn't be very complex to include in the scripts currently under doxygen/), but probably only in a few weeks.

#2 Updated by Roland Schulz over 5 years ago

  1. Remove legacyheaders/ from the include path
    • Pro
      • simpler to setup tooling / IDE (just one path)
      • show immediately all usage of legacyheaders
      • No risk of accidentally working includes (together with using relative paths)
  2. Remove relative paths from public headers
    • Pro
      • reduced accidental complexity ( e.g no need to remember different rules for public/private headers)
      • possible to auto-generate include paths without dependency on CMakeLists.txt (to check for public/private)
      • Headers can be moved with simple automatic change (e.g. sed)
    • Cons
      • Not possible to use headers installed into somethings else than .../gromacs
      • slightly longer path for some headers
      • relative paths don't show difference between public/private headers (still shown by grouping)
  3. Remove relative module-local paths
    • Pro
      • C++ files can be moved without changing includes. Headers can be moved with simple automatic change (e.g. sed)
      • Easier to auto-generate include-paths (e.g. IWYU, generators - path doesn't depend on source location)
    • Cons
      • slightly longer path for headers local to current directory

I don't see extra pros/cons for other misc. relative. I thought your proposal was about 2 not 3.

(Left out short-term disadvantages - don't think they matter on master)

#3 Updated by Roland Schulz over 5 years ago

Can we break down this decision by voting separately on 1, 2, and 3 (as numbered in comment 2)? Or how should we continue?

#4 Updated by Teemu Murtola over 5 years ago

If you want to get progress sooner than later, and it isn't too much effort, it would be great if you could split your change so that you would do at least the first point separately. There should be a clear division between that and the other changes. There's a less clear line between 2 and 3, but something that would cover most of 2 without stepping on 3 that could be easy to automate would be to replace all uses of relative paths starting with ".." (or perhaps all uses of relative paths that are not into the same directory) with an absolute path.

I have an easily customizable include sorter nearly ready, so that could help visualize the different alternatives for the rest. But I don't think we need that to review/accept the first two points.

#5 Updated by Gerrit Code Review Bot over 5 years ago

Gerrit received a related patchset '1' for Issue #1557.
Uploader: Teemu Murtola ()
Change-Id: I362d7c5131a2e6952bbededfd51da4c4e1fc4a7e
Gerrit URL: https://gerrit.gromacs.org/3821

#6 Updated by Gerrit Code Review Bot over 5 years ago

Gerrit received a related patchset '1' for Issue #1557.
Uploader: Teemu Murtola ()
Change-Id: I362d7c5131a2e6952bbededfd51da4c4e1fc4a7e
Gerrit URL: https://gerrit.gromacs.org/3821

#7 Updated by Roland Schulz about 5 years ago

  • Status changed from New to Fix uploaded

#8 Updated by Gerrit Code Review Bot about 5 years ago

Gerrit received a related patchset '1' for Issue #1557.
Uploader: Teemu Murtola ()
Change-Id: Iab3e0cafe09389d32119d72e4cbf4e4f2d929b76
Gerrit URL: https://gerrit.gromacs.org/4008

#9 Updated by Gerrit Code Review Bot about 5 years ago

Gerrit received a related patchset '1' for Issue #1557.
Uploader: Teemu Murtola ()
Change-Id: Iab3e0cafe09389d32119d72e4cbf4e4f2d929b76
Gerrit URL: https://gerrit.gromacs.org/4008

#10 Updated by Teemu Murtola almost 5 years ago

  • Status changed from Fix uploaded to Closed
  • Target version changed from 5.x to 5.1

Now there is some consistency, the rules are documented, and there is enforcement of most of the rules. Anyone dissatisfied can upload changes to all three to Gerrit and we can get change proposals reviewed there.

Also available in: Atom PDF