[PATCH] D18271: Avoid -Wshadow warnings about constructor parameters named after fields

Alexander Kornienko via cfe-commits cfe-commits at lists.llvm.org
Wed Mar 23 18:11:07 PDT 2016

alexfh added a comment.

In http://reviews.llvm.org/D18271#382082, @rnk wrote:

> In http://reviews.llvm.org/D18271#381769, @rsmith wrote:
> > I think a reasonable approach would be: "do not warn on shadowing if the idiom of naming a constructor parameter after a class member is used, and the class member is initialized from that constructor parameter,

Depending on how strictly you define "initialized" here, this is either unhelpful or difficult to chpatterneck. Consider following examples:

  class C1 {
    T field;
    void set_field(T f) {
      field = f;
      // do something else...
    C1(T field) { set_field(field); }
  class C2 {
    T1 f1;
    T2 f2;
    C2(T1 f1, T2 f2) {
      // Some form of initialization of `this->field` from `field` that is not just a copy construction/copy/move,
      // e.g. a method call or an assignment of a single member:
      this->f1.a = f1.a;

I've seen these patterns being used in the real code, and they don't seem to be dangerous or otherwise worth being flagged by this diagnostic.

> > and all uses of the parameter in the constructor body would still be valid if it were declared `const`".


That might also be overly restrictive. Consider a variant of set_field above that takes a `T&` (which is weird, but it doesn't seem like -Wshadow is the right warning in this case).

> That sounds like the right check, but I don't have a lot of time to work on this and I don't want to let the perfect be the enemy of the good. Do you think is a reasonable half-way point for us to leave it for the next year or so?


> > In particular, I think we should probably warn if a non-const reference is bound to the parameter, or if the constructor's member initializer list does not contain `field(field)`.


See above. There are many cases, when at least the latter pattern is used in a reasonable way.


More information about the cfe-commits mailing list