[PATCH] D148712: [clang] Diagnose shadowing of lambda's template parameter by a capture

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.


================
Comment at: clang/lib/Sema/SemaLambda.cpp:1381
+        if (Capture.Id == TP->getIdentifier()) {
+          Diag(Capture.Loc, diag::err_template_param_shadow) << Capture.Id;
+          Diag(TP->getLocation(), diag::note_template_param_here);
----------------
It is really a shame that this is just different enough that we can't turn `CheckRedefinition` into a generic lambda and reuse it here. The code duplication is very unfortunate.


================
Comment at: clang/test/CXX/expr/expr.prim/expr.prim.lambda/expr.prim.lambda.capture/p5.cpp:7
+                                     // expected-note {{variable 'x' is explicitly captured here}}
+  auto h = [y = 0]<typename y>(y) { return 0; };  // expected-error {{declaration of 'y' shadows template parameter}} \
+                                                  // expected-note {{template parameter is declared here}}
----------------
cor3ntin wrote:
> shafik wrote:
> > erichkeane wrote:
> > > Fznamznon wrote:
> > > > shafik wrote:
> > > > > I don't know if shadowing is the correct term to use here. The wording simply says they can't have the same name. I think the diagnostic should say something similar. 
> > > > Well, the example with capture and a parameter uses shadowing term, so I just followed the same approach.
> > > > 
> > > > If we say something like "explicitly captured entity and template parameter can't have the same name", does it make sense to emit a note "captured here" for the capture with conflicting name?
> > > I think the 'shadows' is an appropriate as it matches what we do in normal template cases:
> > > 
> > > ```
> > > template<typename T>
> > > int foo(int T) {
> > >     int T = 5;
> > > }
> > > 
> > > <source>:3:13: error: declaration of 'T' shadows template parameter
> > > int foo(int T) {
> > >             ^
> > > <source>:2:19: note: template parameter is declared here
> > > template<typename T>
> > >                   ^
> > > <source>:4:9: error: redefinition of 'T'
> > >     int T = 5;
> > >         ^
> > > <source>:3:13: note: previous definition is here
> > > int foo(int T) {
> > > ```
> > So [shadowing](https://en.wikipedia.org/wiki/Variable_shadowing) is synonymous w/ name hiding which is not really the case here. I will concede it is consistent with how we are using elsewhere but this PR is probably not the place to change it. So I will drop the objection and probably file a bug report. 
> Well, if it was allowed, it would shadow and it's not allowed.
> I think the message is clear for user - maybe we could ad "which is not allowed" or something, so that it isn't confused with a shadow warning.
Also note the example you gave Erich the ordering is different, the template parameter comes before the function parameter but here it is vice versa, so it feels kind of awkward to say it is shadowing a name that comes after it.


================
Comment at: clang/test/SemaCXX/warn-shadow-in-lambdas.cpp:160
+                                                        // expected-note {{declared here}}
+  auto l3 = [&, y]<typename y, typename>(y) { int a = x; return 0; }; // expected-error {{declaration of 'y' shadows template parameter}} \
+                                                                      // expected-note {{template parameter is declared here}}
----------------
Can we also get a test that verifies this is ok `[]<typename y>(y) { return 0; }` and we obtain no diagnostic.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148712