Skip to content

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

Merged
fanquake merged 2 commits intobitcoin:masterfrom
fjahr:cppcheck17
Dec 2, 2020
Merged

lint, refactor: Update cppcheck linter to c++17 and improve explicit usage#20530
fanquake merged 2 commits intobitcoin:masterfrom
fjahr:cppcheck17

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

practicalswift commented Nov 30, 2020

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

practicalswift commented Dec 2, 2020

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