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

Remove includes in .cpp files for things the corresponding .h file already included #10574

Merged
merged 1 commit into from Dec 12, 2017

Conversation

Projects
None yet
8 participants
@practicalswift
Copy link
Member

practicalswift commented Jun 11, 2017

Remove includes in .cpp files for things the corresponding .h file already included.

Example case:

  • addrdb.cpp includes addrdb.h and fs.h
  • addrdb.h includes fs.h

Then remove the direct inclusion of fs.h in addrman.cpp and rely on the indirect inclusion of fs.h via the included addrdb.h.

In line with the header include guideline (see #10575).

@practicalswift

This comment has been minimized.

Copy link
Member Author

practicalswift commented Jun 12, 2017

Candidates for removal found by:

#!/bin/bash

for H in $(git grep "" -- "*.h" | cut -f1 -d: | uniq); do
    CPP=${H/%\.h/.cpp}
    if [[ ! -e $CPP ]]; then
        continue
    fi
    cat "$H" "$CPP" | grep -E "^#include " | sort | uniq -c | \
      grep " 2 #include" && echo "$CPP" && echo
done

@practicalswift practicalswift force-pushed the practicalswift:redundant branch 3 times, most recently Jun 12, 2017

@practicalswift practicalswift force-pushed the practicalswift:redundant branch Jul 2, 2017

@practicalswift practicalswift force-pushed the practicalswift:redundant branch 2 times, most recently Jul 24, 2017

@practicalswift practicalswift force-pushed the practicalswift:redundant branch Aug 9, 2017

@practicalswift

This comment has been minimized.

Copy link
Member Author

practicalswift commented Aug 9, 2017

Anyone willing to review? :-)

@razamobin

This comment has been minimized.

Copy link

razamobin commented Aug 10, 2017

Hi, I'm new to the bitcoin source code, but I looked for redundant includes and got the same changeset as you. Also I was able to run unit tests and they all passed (although I guess Travis did this already?). Either way, looks good to me.

@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Sep 2, 2017

Soft NACK. IMO it's better code quality to explicitly include everything needed in each source file using them.

@practicalswift practicalswift force-pushed the practicalswift:redundant branch Sep 10, 2017

@practicalswift

This comment has been minimized.

Copy link
Member Author

practicalswift commented Sep 10, 2017

Rebased! :-)

@danra

This comment has been minimized.

Copy link
Contributor

danra commented Sep 10, 2017

I agree with luke-jr

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Sep 10, 2017

@luke-jr @danra Then you should argue to change the guidelines in developer-notes.md:

Every .cpp and .h file should #include every header file it directly uses classes, functions or other definitions from, even if those headers are already included indirectly through other headers. One exception is that a .cpp file does not need to re-include the includes already included in its corresponding .h file.

I personally think that rule makes sense, as you can see the .cpp file as the continuation of the corresponding .h file.

@danra

This comment has been minimized.

Copy link
Contributor

danra commented Sep 10, 2017

@sipa Yes, I'm for changing this rule as it creates an unnecessary dependency of the implementation (.cpp) on the interface (.h). If the interface no longer uses some other include directly, but the implementation still uses that include directly, removing the include from the interface would require adding it to the implementation.

(Note I'm not for having the implementation re-include every include of the interface - only those which it uses directly)

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Sep 10, 2017

@danra I understand that point of view, but I think it's a non-issue. When the interface changes, almost certainly the implementation needs to change anyway.

@practicalswift practicalswift force-pushed the practicalswift:redundant branch Sep 29, 2017

@practicalswift

This comment has been minimized.

Copy link
Member Author

practicalswift commented Sep 29, 2017

Rebased! :-)

@practicalswift practicalswift force-pushed the practicalswift:redundant branch to a720b92 Nov 16, 2017

@practicalswift

This comment has been minimized.

Copy link
Member Author

practicalswift commented Nov 16, 2017

Rebased! Getting this merged would get rid of 76 lines of redundant includes :-)

@laanwj laanwj merged commit a720b92 into bitcoin:master Dec 12, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Dec 12, 2017

Merge #10574: Remove includes in .cpp files for things the correspond…
…ing .h file already included

a720b92 Remove includes in .cpp files for things the corresponding .h file already included (practicalswift)

Pull request description:

  Remove includes in .cpp files for things the corresponding .h file already included.

  Example case:
  * `addrdb.cpp` includes `addrdb.h` and `fs.h`
  * `addrdb.h` includes `fs.h`

  Then remove the direct inclusion of `fs.h` in `addrman.cpp` and rely on the indirect inclusion of `fs.h` via the included `addrdb.h`.

  In line with the header include guideline (see #10575).

Tree-SHA512: 8704b9de3011a4c234db336a39f7d2c139e741cf0f7aef08a5d3e05197e1e18286b863fdab25ae9638af4ff86b3d52e5cab9eed66bfa2476063aa5c79f9b0346
@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Dec 12, 2017

utACK a720b92

#include <QImage>
#include <QPalette>
#include <QPixmap>

This comment has been minimized.

@promag

promag Dec 12, 2017

Member

This is in the header BUT I believe it's not necessary there. IMO the first exercise should be to reduce to a minimal the includes in a header file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.