Skip to content

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Nov 16, 2021

Some of the endif header comments have typos in them (ZMRRPC_H), or are from a different file, or are inconsistently formatted.

Fix all and adjust the linter.

@maflcko maflcko added the Docs label Nov 16, 2021
@katesalazar
Copy link
Contributor

Concept ACK.

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK fa44237

I've reviewed changes in test/lint/lint-include-guards.sh, and verified that all other changes are actually forced by the updated linter.

Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

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

ACK fa44237

The updated lint-include-guard test makes sure that all three keywords: “ifndef”, “define”, “endif //”, must be present for each header file. This also ensures there are no redundant inundation or whitespaces on the lines in which they are present.

I have tested that the updated test is working correctly in following cases:

  1. In case extra space is added anywhere on the line (beginning or in the middle)
  2. Space is removed between “//” and includes guard name.
  3. “//” is changed.

However, it cannot detect if an extra line is added above or below these keywords.

+
 #endif // BITCOIN_LIBSECP256K1_CONFIG_H

Though that’s not the case, this test is designed to handle, so it is not an issue.

@maflcko
Copy link
Member Author

maflcko commented Nov 16, 2021

Though that’s not the case, this test is designed to handle, so it is not an issue.

Correct

@fanquake fanquake merged commit 4a7f6e0 into bitcoin:master Nov 17, 2021
@maflcko maflcko deleted the 2111-libNode branch November 17, 2021 06:39
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 17, 2021
@bitcoin bitcoin locked and limited conversation to collaborators Nov 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants