Skip to content

lint, refactor: Update cppcheck linter to c++17 and improve explicit usage #20530

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Dec 2, 2020
Merged

lint, refactor: Update cppcheck linter to c++17 and improve explicit usage #20530

merged 2 commits into from
Dec 2, 2020

Conversation

fjahr
Copy link
Contributor

@fjahr fjahr commented Nov 29, 2020

I found the extended-lint-cppcheck linter still uses std=c++11 when reviewing #20471. The only difference in the output after this change is one line is missing:

src/script/descriptor.cpp:159:5: warning: Struct 'PubkeyProvider' has a constructor with 1 argument that is not explicit. [noExplicitConstructor]

After some digging, I am still not sure why this one is ignored with c++17 when 40 othernoExplicitConstructor warnings were still appearing.

In the second commit, I fix these warnings, adding explicit where appropriate and adding fixes to ignore otherwise.

@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 30, 2020 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

This was referenced Nov 30, 2020
Merged
Merged
Draft
Draft
Merged
@maflcko
Copy link
Member

maflcko commented Nov 30, 2020

🕵️ @sipa @practicalswift @achow101 have been requested to review this pull request as specified in the {FILENAME_REVIEWERS} file.

@maflcko maflcko mentioned this pull request Nov 30, 2020
Merged
@practicalswift
Copy link
Contributor

Concept ACK: explicit is better than implicit generally, and to be explicit: especially when it comes to explicit ctors

@fjahr Looks like the fuzzing harnesses needs some massage to compile properly after your fix :)

This was referenced Nov 30, 2020
Closed
Merged
Draft
@fjahr
Copy link
Contributor Author

fjahr commented Dec 1, 2020

@fjahr Looks like the fuzzing harnesses needs some massage to compile properly after your fix :)

Thanks, forgot to do that before pushing. Should be ok now.

@practicalswift
Copy link
Contributor

cr ACK 1e62350: patch looks correct!

Rationale behind Concept ACK from C++ Core Guidelines: By default, declare single-argument constructors explicit

@maflcko
Copy link
Member

maflcko commented Dec 2, 2020

review ACK 1e62350

@fanquake fanquake merged commit 0a13d15 into bitcoin:master Dec 2, 2020
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 2, 2020
@fanquake
Merge bitcoin#20530: lint, refactor: Update cppcheck linter to c++17 …
be66fe3 
…and imp…
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@practicalswift practicalswift Awaiting requested review from practicalswift

Assignees
No one assigned
Labels
Refactoring Scripts and tools
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@fjahr @DrahtBot @maflcko @practicalswift @fanquake