google-explicit-constructor warns when explicit(false) is used

[jgopel]

explicit(false) can be seen as a signal that the developer considered the explicit case and determined that, in this instance, explicit is not warranted. Warning when explicit(false) is set forces a NOLINT that duplicates information already available in the syntax. Seen on clang-tidy-14.

struct foo {
    foo(int) {}  // Correctly warns
    explicit foo(long) {}  // Correctly does not warn
    explicit(false) foo(float) {}  // Incorrectly warns
    explicit(true) foo(double) {}  // Correctly does not warn
};

https://godbolt.org/z/4dojdv3s3

[llvmbot]

@llvm/issue-subscribers-clang-tidy

[njames93]

Put https://reviews.llvm.org/D117593 up for review

[llvmbot]

@llvm/issue-subscribers-c-20

[Febbe]

Maybe add also an option to either activate or deactivate the new behavior. Depending on which is chosen as default. This would allow the Google people (and those, which use the style guide) to enforce this.
Also, it would be possible to activate a pipeline test to print a warning, so a reviewer can "accept" the implicit cast.

LGTM so far, but shouldn't all early returns like the following above/before the fixup/message construction?

  if (Ctor->isExplicit() || Ctor->isCopyOrMoveConstructor() ||
      TakesInitializerList || Ctor->getExplicitSpecifier().isSpecified())
    return;

[njames93]

Maybe I should add an option instead. This does lead highlight another issue with the check not having support for these explicit specifiers. Currently the fix emitted is like this

explict(false) CTor(int) {}

Transformed to

explicit(false) explicit Ctor(int) {}

Instead it should either be

explicit(true) Ctor(int) {}
// Or more favourable
explicit Ctor(int) {}

[jgopel]

FWIW: I prefer explicit(true) to explicit - an option for the fixer to do explicit(true) instead of explicit would allow the user to select which is the preferred style.