-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Lint fixes #4849
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
Lint fixes #4849
Conversation
Years are set according to 'git log' history Zcash: Only the scripts we have that are missing the header.
These are text files but their encoding does not depend on the locale. Not all of them require utf8 but it is better to fix it at something to remove potential unpredictability. This is necessary on FreeBSD where no locale is set by default, and apparently Python defaults not only the terminal encoding to the locale but that of every text file. So without LOCALE environment it defaults text file encoding to ASCII. This causes problems with e.g. `bitcoin.conf`. Luckily the locale doesn't affect the default encoding for str.encode() and bytes.decode() on Python 3, so this is the only change necessary.
Instead of calling sprintf for every byte, format the hex bytes ourselves by help of HexStr and a reverse_iterator.
Zcash: Only the script we have.
We have several pieces of information about subtrees: 1) What their current directory contents is 2) What their directory contents was at the time of the last subtree merge 3) What the directory contents of the upstream project is in the commit referred to by the subtree merge. Normally, all 3 should be identical. git-subtree-check.sh so far only compared (1) with (3) however. Fix this by comparing all three, and give some more useful diff output in the case of mismatch. The added benefit is that (1) and (2) can be compared without needing to see the upstream repository.
This partially reverts commit ab8e8b9
git-subtree-check fails if the directory is given with a trailing slash, eg: ``` > test/lint/git-subtree-check.sh src/univalue/ ERROR: src/univalue/ is not a subtree ``` Shell autocompletes will add the trailing slash when autofilling the path name, which will therefore cause the script to fail. Just ignore any trailing slash.
These are external libraries, and it does not make sense to maintain an otherwise-meaningless diff from upstream. This partially reverts commit 1e6d183.
Fixes a false positive match to the locale-dependent trim function.
|
Of the lints that are not fixed in this PR, many are addressed upstream in the arg-parsing and Python 3 refactors, which I want to backport in separate PRs. The only remaining lints in code we introduced are:
|
daira
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK with comments.
| #!/bin/bash | ||
| # Copyright (c) 2013 The Bitcoin Core developers | ||
| # Distributed under the MIT software license, see the accompanying | ||
| # file COPYING or http://www.opensource.org/licenses/mit-license.php. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be an https URL, and similarly for other cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to address these in another PR. We're going to keep getting these from upstream PRs, so may as well change them periodically in bulk.
| HEADER_ID_SUFFIX="_H" | ||
|
|
||
| REGEXP_EXCLUDE_FILES_WITH_PREFIX="src/(leveldb/|secp256k1/|univalue/)" | ||
| REGEXP_EXCLUDE_FILES_WITH_PREFIX="src/(crypto/ctaes/|leveldb/|secp256k1/|tinyformat.h|univalue/)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these excluded rather than just fixed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per the commit message:
lint: Re-exclude subtrees from lint-include-guards.sh
These are external libraries, and it does not make sense to maintain an
otherwise-meaningless diff from upstream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For more meaningful lint changes (like locale, or the changes we've made to secp256k1), sure. The include guards have zero impact on our codebase (they've been working fine as-is), so it just adds noise.
We don't maintain any of the current subtrees, but we might in future. Co-authored-by: Daira Hopwood <[email protected]>
|
@zkbot r+ |
|
📌 Commit e531d72 has been approved by |
Fixes most lints currently reported by
test/lint/lint-all.sh.Includes changes cherry-picked from the following upstream PRs: