[PATCH] D153536: [Clang] Implement P2169 A nice placeholder with no name

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin marked 14 inline comments as done.
cor3ntin added inline comments.


================
Comment at: clang/include/clang/AST/DeclBase.h:340-342
+  /// Whether this declaration denotes a placeholder that can be
+  /// redefined in the same scope.
+  unsigned IsPlaceholder : 1;
----------------
aaron.ballman wrote:
> cor3ntin wrote:
> > aaron.ballman wrote:
> > > This seems a bit heavy to put on every `Decl`, it only matters for declarations with names, right? Should this be on `NamedDecl` instead? And should it be using terminology like "name independent"?
> > > 
> > > Actually, do we need this bit at all? Would it work for `NamedDecl` to have a function that returns whether the declaration name is `_` or not? Oh, I suppose we can't get away with that because `void _(int x);` does not introduce a placeholder identifier but a regular one, right?
> > I think you made me realize we can probably compute that completely without using bits. Oups.
> > It would certainly simplify things. I'll investigate!
> Thank you!
Removed!


================
Comment at: clang/test/SemaCXX/cxx2c-placeholder-vars.cpp:31
+                           // expected-note 4{{placeholder declared here}} \\
+                           // expected-warning 2{{placeholder variable has no side effect}}
+        (void)_++; // expected-error {{referring to placeholder '_' is not allowed}}
----------------
aaron.ballman wrote:
> cor3ntin wrote:
> > aaron.ballman wrote:
> > > I don't understand what this diagnostic is trying to tell me, can you explain a bit more?
> > The no side effect one?
> > You are assigning something that has no side effect to something that you may never be able to name, so the whole thing is likely dead code.
> > I welcome improvements suggestions
> Yeah, the no side effect one.
> 
> Corentin and I discussed this off-list and I understand the situation a bit better now. Basically, an init capture named `_` either has a side effect on construction and/or destruction (like an RAII object) or it cannot be used within the lambda. However, it does still have effects (for example, it impacts whether the lambda has a function conversion operator).
> 
> That makes this kind of tricky to word. Given that it's a QoI warning, I'd recommend we leave this bit for a follow-up so that we can land the majority of the functionality and we can add the safety rails later.
Removed


================
Comment at: clang/test/SemaCXX/cxx2c-placeholder-vars.cpp:79
+void test_param(int _) {}
+void test_params(int _, int _); // expected-error {{redefinition of parameter '_'}} \
+                                // expected-note {{previous declaration is here}}
----------------
aaron.ballman wrote:
> cor3ntin wrote:
> > aaron.ballman wrote:
> > > Weird.. is this expected? You can have two variables named `_` in block scope, but you can't have two parameters named `_` despite them inhabiting the same block scope?
> > Yes, CWG decided against having it in parameter declarations, because otherwise we'd need them in template parameters which we didn't want to do.
> > And it would be useless, you can simply not name them.
> I guess I see the same amount of utility in allowing an init capture named `_` as I do a function parameter named `_` as a local variable named `_` (if it is an RAII type, it can do useful things in theory; otherwise, it's not a useful declaration).
side effects are the main motivation for local variables beside structured bindings (and pattern matching later)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153536