[cfe-dev] Add a clang-tidy check for inadvertent conversions

[Krzemienski, Andrzej via cfe-dev]

-----Original Message-----
From: Roman Lebedev [mailto:lebedev.ri at gmail.com] 
Sent: Monday, March 19, 2018 1:50 PM
To: Krzemienski, Andrzej <Andrzej.Krzemienski at sabre.com>
Cc: cfe-dev at lists.llvm.org
Subject: Re: [cfe-dev] Add a clang-tidy check for inadvertent conversions

On Mon, Mar 19, 2018 at 2:40 PM, Krzemienski, Andrzej via cfe-dev <cfe-dev at lists.llvm.org> wrote:
> > Hi Everyone,
> > I am considering contributing a clang-tidy check that will also require changes to clang sources. I would like to solicit feedback from people in this list. Here is the idea.
>
> > The goal is to detect constructors unintentionally defined as converting (when someone has forgotten to declare one's constructor as explicit).
> > I want to add an attribute, call it, say, [[conversion]] or [[implicit]]. It applies only to constructors. I want to add another check to clang tidy, say cppcoreguidelinse-explicit-ctor (referring to http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-explicit).
> > The warning is triggered when:
>
> > 1. We see a declaration of non-copy, non-move constructor that can be called with a single argument and it is neither declared explicit not annotated with attribute [[conversion]].
>
> > 2. We see a declaration of a copy ctor or a move ctor or a constructor that cannot be called with a single argument which annotated with attribute [[conversion]].
>
> > Example:
>
> > ```
> > Struct X
> > {
> >   X(X const&); // ok: copy-ctor is special
> >   X(int I, int j, int k); // ok: cannot be called with one arg
> >   X(int i, int j = 0); // warn: non-copy, non-move, non-explicit, can be called with one arg
> >   template <typename... T> > X(T&&... args); // warn: can be called with 1 argument
> >   explicit X(double); // ok: explicit
> >   X(char) [[conversion]]; // ok: warning disabled by attribute
>
> >   explicit X(std::string) [[conversion]]; // warn: attribute is redundant
> >   X(char, char) [[conversion]]; // warn: attribute is redundant }; ```
> FWIW there already is a clang-tidy google-explicit-constructor check, https://clang.llvm.org/extra/clang-tidy/checks/google-explicit-constructor.html
> and one can silence the selected cases with  // NOLINT And with newest clang-tidy versions support //
> NOLINT(google-explicit-constructor)
> (See http://clang.llvm.org/extra/clang-tidy/ )

> So i may have missed something, but i think the main idea is already supported, although not as pretty..

Thanks, for bringing this up. Yes, in a way it does address the same problem, although I feel using it is more like a workaround rather than a proper solution.

Google's policy (and the corresponding check) is to consider any converting constructor a bug (or a problem, or whatever we want to call it). What I am after is to warn only about conversions that occur inadvertently, but not in the code where it is the programmers intention to provide the conversion. Warning on conversion operator (the Google check does this also) is something I definitely do not want.

It would be too much false positives to NOLINT.


> > Proposed implementation:
> > Currently the AST matchers are not capable of extracting information from user-defined attributes. Implementing such extraction is beyond my capacity. Instead, I could teach clang to recognize one more attribute, this [[conversion]], the same way as it recognizes [[noreturn]]. Hen adding a clang-tidy check would be quite trivial.