Skip to content

Clarify include guard naming convention#12757

Merged
maflcko merged 3 commits intobitcoin:masterfrom
practicalswift:include-guard
Apr 1, 2018
Merged

Clarify include guard naming convention#12757
maflcko merged 3 commits intobitcoin:masterfrom
practicalswift:include-guard

Conversation

@practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Mar 22, 2018

  • Documentation: Document include guard convention
  • Fix: Fix missing or inconsistent include guards
  • Regression test: Add lint-include-guards.sh which checks include guard consistency

@practicalswift practicalswift force-pushed the include-guard branch 4 times, most recently from 1231374 to ff981df Compare March 22, 2018 15:42
@promag
Copy link
Contributor

promag commented Mar 22, 2018

Concept ACK.

Could run script in travis where CHECK_DOC=1 to prevent future PR's breaking it?

@practicalswift
Copy link
Contributor Author

practicalswift commented Mar 22, 2018 •
edited
Loading

@promag contrib/devtools/lint-*.sh is executed automatically by Travis (via lint-all.sh which is run as part of CHECK_DOC=1). In other words contrib/devtools/lint-include-guards.sh will be executed by Travis – no need to reference it explicitly :-)

eklitzke
eklitzke reviewed Mar 22, 2018
View reviewed changes
Copy link
Contributor

@eklitzke eklitzke left a comment

Choose a reason for hiding this comment

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

Concept ACK

contrib/devtools/lint-include-guards.sh Outdated
Copy link
Contributor

@eklitzke eklitzke Mar 22, 2018

Choose a reason for hiding this comment

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

Do you really need bitcoin-config.h and build.h in here? They're auto-generated so I would have thought git ls-files wouldn't have listed them.

contrib/devtools/lint-include-guards.sh Outdated
Copy link
Contributor

@eklitzke eklitzke Mar 22, 2018

Choose a reason for hiding this comment

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

You should probably add a copyright notice to this file.

@practicalswift
Copy link
Contributor Author

practicalswift commented Mar 22, 2018

@eklitzke Thanks for reviewing! Feedback addressed. Please re-review :-)

@sipa
Copy link
Member

sipa commented Mar 22, 2018

utACK 3bcc005

@maflcko maflcko closed this Mar 22, 2018
@maflcko maflcko reopened this Mar 22, 2018
promag
promag reviewed Mar 22, 2018
View reviewed changes
contrib/devtools/lint-include-guards.sh
REGEXP_EXCLUDE_FILES_WITH_PREFIX="src/(crypto/ctaes/|leveldb/|secp256k1/|tinyformat.h|univalue/)"

EXIT_CODE=0
for HEADER_FILE in $(git ls-files -- "*.h" | grep -vE "^${REGEXP_EXCLUDE_FILES_WITH_PREFIX}")
Copy link
Contributor

@promag promag Mar 22, 2018

Choose a reason for hiding this comment

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

Could use git ls-files -x instead of | grep -vE?

Copy link
Contributor

@promag promag Mar 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Never mind, -x skips untracked files.

Copy link
Contributor Author

@practicalswift practicalswift Mar 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@promag No I don't think so (-x is for untracked files, right?) :-)

I'd be glad to be proven wrong so provide an equivalent using git ls-files -x and I'll switch :-)

Copy link
Contributor

@promag promag Mar 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Yes I've come to that conclusion right after trying it.

@promag
Copy link
Contributor

promag commented Mar 22, 2018

Tested ACK 3bcc005 on mac.

eklitzke
eklitzke approved these changes Mar 27, 2018
View reviewed changes
Copy link
Contributor

@eklitzke eklitzke left a comment

Choose a reason for hiding this comment

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

Thanks! utACK 3bcc005

@maflcko maflcko merged commit 3bcc005 into bitcoin:master Apr 1, 2018
maflcko pushed a commit that referenced this pull request Apr 1, 2018
Merge #12757: Clarify include guard naming convention
0c5f67b 
3bcc005 Add lint-include-guards.sh which checks include guard consistency (practicalswift)
8fd6af8 Fix missing or inconsistent include guards (practicalswift)
8af65d9 Document include guard convention (practicalswift)

Pull request description:

  * **Documentation**: Document include guard convention
  * **Fix**: Fix missing or inconsistent include guards
  * **Regression test**: Add `lint-include-guards.sh` which checks include guard consistency

Tree-SHA512: 8171878f60fd08ccbea943a11e835195750592abb9d7ab74eaa4265ae7fac523b1da9d31ca13d6ab73dd596e49986bfb7593c696e5f39567c93e610165bc2acc
str4d added a commit to str4d/zcash that referenced this pull request May 4, 2018
@str4d
Add missing include guard
768cd14 
Extracted from bitcoin/bitcoin#12757
- Commit 8fd6af8
@str4d str4d mentioned this pull request May 4, 2018
Merged
zkbot added a commit to zcash/zcash that referenced this pull request May 7, 2018
@zkbot
Auto merge of #3202 - str4d:3058-sapling-bech32, r=<try>
19ac942 
Bech32 encoding support

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#8578
- bitcoin/bitcoin#11167
  - Only the second and third commits (first is in #3228, fourth depends on #2390, later ones are SegWit-specific).
- bitcoin/bitcoin#12757
  - Only the change to `src/bech32.h`

Part of #3058.
zkbot added a commit to zcash/zcash that referenced this pull request May 8, 2018
@zkbot
Auto merge of #3202 - str4d:3058-sapling-bech32, r=str4d
b1d2a69 
Bech32 encoding support

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#8578
- bitcoin/bitcoin#11167
  - Only the second and third commits (first is in #3228, fourth depends on #2390, later ones are SegWit-specific).
- bitcoin/bitcoin#12757
  - Only the change to `src/bech32.h`

Part of #3058.
@sickpig sickpig mentioned this pull request May 5, 2020
Merged
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 19, 2020
@PastaPastaPasta
Merge bitcoin#12757: Clarify include guard naming convention
db747ea 
3bcc005 Add lint-include-guards.sh which checks include guard consistency (practicalswift)
8fd6af8 Fix missing or inconsistent include guards (practicalswift)
8af65d9 Document include guard convention (practicalswift)

Pull request description:

  * **Documentation**: Document include guard convention
  * **Fix**: Fix missing or inconsistent include guards
  * **Regression test**: Add `lint-include-guards.sh` which checks include guard consistency

Tree-SHA512: 8171878f60fd08ccbea943a11e835195750592abb9d7ab74eaa4265ae7fac523b1da9d31ca13d6ab73dd596e49986bfb7593c696e5f39567c93e610165bc2acc
Signed-off-by: pasta <[email protected]>

# Conflicts:
#	src/bech32.h
#	src/consensus/merkle.h
#	src/key_io.h
#	src/policy/fees.h
#	src/rpc/server.h
#	src/script/bitcoinconsensus.h
#	src/wallet/coinselection.h
@str4d str4d mentioned this pull request Oct 27, 2020
Merged
zkbot added a commit to zcash/zcash that referenced this pull request Oct 28, 2020
@zkbot
Auto merge of #4829 - str4d:backport-lints, r=<try>
43ac206 
Backport useful lints from upstream

Cherry-picked from the following upstream PRs:
- bitcoin/bitcoin#6892
- bitcoin/bitcoin#11151
- bitcoin/bitcoin#11300
- bitcoin/bitcoin@96d91b7
- bitcoin/bitcoin#12097
- bitcoin/bitcoin#12098
- bitcoin/bitcoin#12442
- bitcoin/bitcoin#12572
- bitcoin/bitcoin#12757
- bitcoin/bitcoin#11878
- bitcoin/bitcoin#12933
- bitcoin/bitcoin#12871
- bitcoin/bitcoin#12972
- bitcoin/bitcoin#13281
- bitcoin/bitcoin#13385
- bitcoin/bitcoin#13041
- bitcoin/bitcoin#13454
- bitcoin/bitcoin#13448
- bitcoin/bitcoin#13510
- bitcoin/bitcoin#13851
- bitcoin/bitcoin#13863
- bitcoin/bitcoin#14115
- bitcoin/bitcoin#14831
- bitcoin/bitcoin#15164
- bitcoin/bitcoin#15170
- bitcoin/bitcoin#15166
- bitcoin/bitcoin#16036
- bitcoin/bitcoin#16768

Several of the lints fail for our current codebase; these will be addressed in a
subsequent PR.
zkbot added a commit to zcash/zcash that referenced this pull request Nov 9, 2020
@zkbot
Auto merge of #4829 - str4d:backport-lints, r=str4d
84a5830 
Backport useful lints from upstream

Cherry-picked from the following upstream PRs:
- bitcoin/bitcoin#6892
- bitcoin/bitcoin#11151
- bitcoin/bitcoin#11300
- bitcoin/bitcoin@96d91b7
- bitcoin/bitcoin#12097
- bitcoin/bitcoin#12098
- bitcoin/bitcoin#12442
- bitcoin/bitcoin#12572
- bitcoin/bitcoin#12757
- bitcoin/bitcoin#11878
- bitcoin/bitcoin#12933
- bitcoin/bitcoin#12871
- bitcoin/bitcoin#12972
- bitcoin/bitcoin#13281
- bitcoin/bitcoin#13385
- bitcoin/bitcoin#13041
- bitcoin/bitcoin#13454
- bitcoin/bitcoin#13448
- bitcoin/bitcoin#13510
- bitcoin/bitcoin#13851
- bitcoin/bitcoin#13863
- bitcoin/bitcoin#14115
- bitcoin/bitcoin#14831
- bitcoin/bitcoin#15164
- bitcoin/bitcoin#15170
- bitcoin/bitcoin#15166
- bitcoin/bitcoin#16036
- bitcoin/bitcoin#16768
- bitcoin/bitcoin#13494

Several of the lints fail for our current codebase; these will be addressed in a
subsequent PR.
@practicalswift practicalswift deleted the include-guard branch April 10, 2021 19:33
gades pushed a commit to cosanta/cosanta-core that referenced this pull request Mar 3, 2022
@gades
Merge bitcoin#12757: Clarify include guard naming convention
cc45d2d 
3bcc005 Add lint-include-guards.sh which checks include guard consistency (practicalswift)
8fd6af8 Fix missing or inconsistent include guards (practicalswift)
8af65d9 Document include guard convention (practicalswift)

Pull request description:

  * **Documentation**: Document include guard convention
  * **Fix**: Fix missing or inconsistent include guards
  * **Regression test**: Add `lint-include-guards.sh` which checks include guard consistency

Tree-SHA512: 8171878f60fd08ccbea943a11e835195750592abb9d7ab74eaa4265ae7fac523b1da9d31ca13d6ab73dd596e49986bfb7593c696e5f39567c93e610165bc2acc
Signed-off-by: pasta <[email protected]>
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@promag promag promag left review comments

@eklitzke eklitzke eklitzke approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@practicalswift @promag @sipa @eklitzke @maflcko