[PATCH] D117593: [clang-tidy] Change google-explicit-constructor to ignore conversions and operators marked `explicit(false)`

Aaron Ballman via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Jan 28 08:41:06 PST 2022


aaron.ballman added a comment.

In D117593#3274408 <https://reviews.llvm.org/D117593#3274408>, @njames93 wrote:

> In D117593#3273562 <https://reviews.llvm.org/D117593#3273562>, @aaron.ballman wrote:
>
>> Hmmm, this is a rule for checking a specific style guide. https://google.github.io/styleguide/cppguide.html#Implicit_Conversions doesn't say that `explicit(false)` is a reasonable marking. In fact, it says "Implicit conversions can sometimes be necessary and appropriate for types that are designed to be interchangeable, for example when objects of two types are just different representations of the same underlying value. In that case, contact your project leads to request a waiver of this rule."
>>
>> So I think this behavior needs to be behind a flag so that the default behavior continues to match what the style guide requires (or the style guide should be updated to clarify the behavior of `explicit(false)`).
>
> My understanding of the rule(as a non Googler) was that `explicit(false)` is an effective way to disable the rule by explicitly declaring the constructor to be implicit. Which is much cleaner than throwing `NOLINT` markers about.

FWIW, I agree that this would be a reasonable exception to the rule and makes for cleaner code than `NOLINT` comments. I'm pointing out that the rule text doesn't say that exceptions are something they want to silence diagnostics for. The way I read the guideline suggests that they don't consider the diagnostic a false positive.

> In any case this c++20 syntax is not supported as the current fixit produces invalid code, as evidenced in the initial bug report.

Agreed that issue needs to be fixed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117593/new/

https://reviews.llvm.org/D117593



More information about the cfe-commits mailing list