Skip to content

test: Add linter to make sure single parameter constructors are marked explicit #14505

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
laanwj merged 1 commit into from
Jul 8, 2019
Merged

test: Add linter to make sure single parameter constructors are marked explicit #14505

laanwj merged 1 commit into from
Jul 8, 2019

Conversation

@practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Oct 17, 2018 •
edited
Loading

Make single parameter constructors explicit (C++11).

Rationale from the developer notes:

  • By default, declare single-argument constructors explicit.
    • Rationale: This is a precaution to avoid unintended conversions that might
      arise when single-argument constructors are used as implicit conversion
      functions.

@promag
Copy link
Contributor

promag commented Oct 18, 2018

Concept ACK.

@practicalswift is there a compiler flag to detect these?

@practicalswift
Copy link
Contributor Author

practicalswift commented Oct 19, 2018

@promag Perhaps surprisingly clang and GCC lack such a warning AFAIK, but the Intel C++ Compiler has it.

Also most C++ linters support flagging single parameter constructors not declared explicit.

@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 20, 2018 •
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:

  • #13728 (lint: Run the CI lint stage on mac by Empact)

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 Oct 20, 2018
Merged
Merged
@maflcko
Copy link
Member

maflcko commented Oct 20, 2018

See #13769, #11112, #10969, ...

At this point I'd prefer a linter that runs in travis than having a fixup pull request every couple of weeks. I think there are some intentional cases of not using explicit, so those could be added to a whitelist.

@practicalswift practicalswift force-pushed the explicit-single-argument-constructors branch 2 times, most recently from 95d1431 to 8113aeb Compare October 21, 2018 14:28
@practicalswift practicalswift force-pushed the explicit-single-argument-constructors branch from 8113aeb to dcd77cc Compare October 23, 2018 03:41
This was referenced Oct 23, 2018
Merged
Merged
Merged
@practicalswift practicalswift force-pushed the explicit-single-argument-constructors branch 6 times, most recently from 934811f to 1236d01 Compare November 7, 2018 16:30
@practicalswift
Copy link
Contributor Author

practicalswift commented Nov 7, 2018 •
edited
Loading

@MarcoFalke Good point! I've now added a Travis check that will catch newly introduced single-argument constructors not declare explicit. Please re-review :-)

@practicalswift practicalswift force-pushed the explicit-single-argument-constructors branch from 710e02a to 5a4b5fa Compare November 7, 2018 17:49
@practicalswift practicalswift changed the title Make all single parameter constructors explicit (C++11) Make all single parameter constructors explicit (C++11). Add linter. Nov 11, 2018
@practicalswift practicalswift changed the title Make all single parameter constructors explicit (C++11). Add linter. Make all single parameter constructors explicit (C++11). Add explicit constructor linter. Nov 11, 2018
@practicalswift practicalswift force-pushed the explicit-single-argument-constructors branch from 5a4b5fa to c191011 Compare November 12, 2018 14:24
@practicalswift
Copy link
Contributor Author

practicalswift commented Nov 12, 2018

@promag Would you mind reviewing the updated version which contains the automatic Travis checking for explicit?

If merged we'll never have to think about explicit ever again :-)

@Empact
Copy link
Contributor

Empact commented Nov 22, 2018 •
edited
Loading

utACK c191011 - could squash the first five commits, and "all" is not strictly accurate given the linter exceptions.

@practicalswift practicalswift mentioned this pull request Feb 26, 2019
Closed
@practicalswift
Copy link
Contributor Author

practicalswift commented Mar 6, 2019

@MarcoFalke Can this one get a release milestone? :-)

@maflcko
Copy link
Member

maflcko commented Mar 6, 2019

This is controversial in its current form. See #14505 (comment)

I guess I'd be ok with it if it would never fail (and only print the warnings), like the spell-checker.

maflcko
maflcko reviewed Mar 6, 2019
View reviewed changes
@maflcko maflcko changed the title Add linter to make sure single parameter constructors are marked explicit (C++11) by default test: Add linter to make sure single parameter constructors are marked explicit Mar 6, 2019
@practicalswift practicalswift force-pushed the explicit-single-argument-constructors branch 2 times, most recently from 6871f37 to 2eb79c3 Compare March 6, 2019 21:50
@practicalswift
Copy link
Contributor Author

practicalswift commented Mar 6, 2019

@MarcoFalke Updated. The updated version never fails: it only prints the warnings -- like the spell-checker. Looks good? :-)

@maflcko
Copy link
Member

maflcko commented Mar 6, 2019

utACK 2eb79c3601bfb4bff140693b3225aa4779d73390

Empact
Empact reviewed Mar 13, 2019
View reviewed changes
@Empact
Copy link
Contributor

Empact commented Mar 13, 2019

utACK 2eb79c3

@practicalswift practicalswift force-pushed the explicit-single-argument-constructors branch from 48fda19 to 64b0cf1 Compare March 20, 2019 13:34
@practicalswift
Copy link
Contributor Author

practicalswift commented Mar 20, 2019

Fixed typo identified by @Empact and squashed.

@Empact @MarcoFalke @promag Would you mind re-reviewing? :-)

@practicalswift practicalswift force-pushed the explicit-single-argument-constructors branch from 64b0cf1 to c4606b8 Compare June 26, 2019 14:57
@laanwj
Copy link
Member

laanwj commented Jul 8, 2019

ACK c4606b8

@laanwj laanwj merged commit c4606b8 into bitcoin:master Jul 8, 2019
laanwj added a commit that referenced this pull request Jul 8, 2019
@laanwj
Merge #14505: test: Add linter to make sure single parameter construc…
345f42a 
…tors are marked explicit

c4606b8 Add Travis check for single parameter constructors not marked "explicit" (practicalswift)

Pull request description:

  Make single parameter constructors `explicit` (C++11).

  Rationale from the developer notes:

  > - By default, declare single-argument constructors `explicit`.
  >   - *Rationale*: This is a precaution to avoid unintended conversions that might
  >   arise when single-argument constructors are used as implicit conversion
  >   functions.

ACKs for top commit:
  laanwj:
    ACK c4606b8

Tree-SHA512: 3e6fd51935fd93b2604b2188664692973d0897469f814cd745b5147d71b99ea5d73c1081cfde9f6393f51f56969e412fcda35d2d54e938a3235b8d40945f31fd
maflcko
maflcko reviewed Jul 8, 2019
View reviewed changes
test/lint/extended-lint-cppcheck.sh
echo "Advice not applicable in this specific case? Add an exception by updating"
echo "IGNORED_WARNINGS in $0"
# Uncomment to enforce the developer note policy "By default, declare single-argument constructors `explicit`"
# exit 1
Copy link
Member

@maflcko maflcko Jul 8, 2019

Choose a reason for hiding this comment

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

It could be enabled, but guarded by a allow_failures travis flag?

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 9, 2019
@laanwj @sidhujag
Merge bitcoin#14505: test: Add linter to make sure single parameter c…
50942d0 
…onstructors are marked explicit

c4606b8 Add Travis check for single parameter constructors not marked "explicit" (practicalswift)

Pull request description:

  Make single parameter constructors `explicit` (C++11).

  Rationale from the developer notes:

  > - By default, declare single-argument constructors `explicit`.
  >   - *Rationale*: This is a precaution to avoid unintended conversions that might
  >   arise when single-argument constructors are used as implicit conversion
  >   functions.

ACKs for top commit:
  laanwj:
    ACK c4606b8

Tree-SHA512: 3e6fd51935fd93b2604b2188664692973d0897469f814cd745b5147d71b99ea5d73c1081cfde9f6393f51f56969e412fcda35d2d54e938a3235b8d40945f31fd
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jun 12, 2020
@practicalswift @Fabcien
Fix single parameter constructors not marked "explicit"
09768be 
Summary:
This is a backport of core [[bitcoin/bitcoin#14505 | PR14505]], but the linter part will be set in
another diff.

Test Plan:
  ninja all check

Reviewers: #bitcoin_abc, deadalnix

Reviewed By: #bitcoin_abc, deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D6547
@practicalswift practicalswift deleted the explicit-single-argument-constructors branch April 10, 2021 19:38
Munkybooty pushed a commit to Munkybooty/dash that referenced this pull request Nov 24, 2021
@laanwj @Munkybooty
Merge bitcoin#14505: test: Add linter to make sure single parameter c…
d58ff4e 
…onstructors are marked explicit

c4606b8 Add Travis check for single parameter constructors not marked "explicit" (practicalswift)

Pull request description:

  Make single parameter constructors `explicit` (C++11).

  Rationale from the developer notes:

  > - By default, declare single-argument constructors `explicit`.
  >   - *Rationale*: This is a precaution to avoid unintended conversions that might
  >   arise when single-argument constructors are used as implicit conversion
  >   functions.

ACKs for top commit:
  laanwj:
    ACK c4606b8

Tree-SHA512: 3e6fd51935fd93b2604b2188664692973d0897469f814cd745b5147d71b99ea5d73c1081cfde9f6393f51f56969e412fcda35d2d54e938a3235b8d40945f31fd
@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

@fanquake fanquake fanquake left review comments

+2 more reviewers

@Empact Empact Empact left review comments

@maflcko maflcko maflcko left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@practicalswift @promag @DrahtBot @maflcko @Empact @luke-jr @laanwj @fanquake