[PATCH] D30547: [clang-tidy] Forwarding reference overload in constructors

Aaron Ballman via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Mar 22 07:01:42 PDT 2017


aaron.ballman added inline comments.


================
Comment at: clang-tidy/misc/ForwardingReferenceOverloadCheck.cpp:125-126
+    }
+    diag(Ctor->getLocation(), "function %0 can hide copy and move constructors")
+        << Ctor;
+  }
----------------
xazax.hun wrote:
> aaron.ballman wrote:
> > aaron.ballman wrote:
> > > leanil wrote:
> > > > aaron.ballman wrote:
> > > > > I think a better diagnostic might be: "constructor accepting a universal reference hides the %select{copy|move|both the copy and move}0 %select{constructor{|s}1"
> > > > > 
> > > > > And then provide a note ("%select{copy|move}0 constructor declared here") that points to the offending copy and/or move constructor.
> > > > Without checking actual constructor calls, I would have to make notes on every (non disabled) copy / move constructor, any time I produce a warning. And as the warning already states where the problem lies, the notes would only help people find the copy and move constructors. Do you think that's necessary? 
> > > The warning states where the forwarding reference constructor is, but it doesn't state where the conflicting constructors are. When we issue diagnostics like that, we generally use notes so that the user can see all of the locations involved -- the user may want to get rid of the other constructors, or they may want to get rid of the forwarding reference constructor. Also, saying "can hide" implies that it isn't hiding anything at all, just that it's possible to do so. Tightening up the wording and showing all of the locations involved solves both problems.
> > This isn't quite complete. It's still an ambiguous statement to say "it can hide"; it does hide these constructors, and we even know which ones. Emit the notes before you emit the main diagnostic and you can use the `%select` suggested originally to be specific in the diagnostic.
> We can not say for sure without looking at a concrete call whether a constructor is "hidden" or not. It is always determined during the overload resolution.
> 
> This check does not consider the calls, because that way it would always miss the possible misuses if libraries. 
I can see the logic in that. I guess I'm thinking of it the same way we use the phrase "hidden" when describing code like:
```
struct Base {
  virtual void f(int);
};

struct Derived : Base {
  void f(double);
};

```
We claim Derived::f() hides Base::f() without considering the callers.


Repository:
  rL LLVM

https://reviews.llvm.org/D30547





More information about the cfe-commits mailing list