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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/docs/ReleaseNotes.rst:135-136
 - Compiler flags ``-std=c++2c`` and ``-std=gnu++2c`` have been added for experimental C++2c implementation work.
+- Implemented `P2169R4: A nice placeholder with no name <https://wg21.link/P2169R4>`_. This allows to use `_`
+  as a variable name multiple times in the same scope and is supported in all C++ language modes as an extension.
 
----------------
cor3ntin wrote:
> aaron.ballman wrote:
> > Should we consider adding this as an extension to C? It would be conforming there as well because it wouldn't change the behavior of any correct C code, right?
> Maybe we should consult WG14 first?
> It's of very questionable usefulness without structured bindings and destructors.
We certainly could ask WG14 for their opinion, but the scenario I was thinking this would help with is for complex macros where variables are sometimes defined but the names are not important as they shouldn't escape. However, I think those scenarios generally introduce different scopes (often with GNU statement expressions) so there's not a naming conflict that we'd need to avoid. So I'm fine skipping C for now until we hit a concrete use case.


================
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;
----------------
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!


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:6589
+def err_using_placeholder_variable : Error<
+  "referring to placeholder '_' is not allowed">;
+def note_reference_placeholder : Note<
----------------
cor3ntin wrote:
> cjdb wrote:
> > aaron.ballman wrote:
> > > I don't think this helps the user understand what's wrong with their code, especially given the somewhat odd language rules around the feature. How about: `ambiguous reference to multiply-defined placeholder '_'` or something along those lines? Then the note can show the previous declarations of the placeholders that are in scope? e.g.,
> > > ```
> > > void g() {
> > > int _; // note: placeholder declared here
> > > _ = 0;
> > > int _; // note: placeholder declared here
> > > _ = 0; // error: `ambiguous reference to multiply-defined placeholder '_'`
> > > }
> > > ```
> > > CC @cjdb 
> > Agreed. I'd suggest a rewording though: I took "multiply" to mean the maths term until completing the sentence, rather than its alternative meaning of "multiple instances" (which is more or less the same meaning, but "multiply" maps to the `x * y` operation for me).
> > 
> > Perhaps `ambiguous reference to placeholder '_', which has multiple definitions`? Not sold on that being the best wording, but it does avoid the hardcoded-word-at-8yo problem :)
> @cjdb I like your suggestion. maybe "which is defined multiple times?"  - pedantically a bunch of things  have been defined once each, it's not a redefinition of the same entity in the c++ sense
I like either of your suggestions better than my use of "multiply-defined." :-)


================
Comment at: clang/lib/Sema/SemaDecl.cpp:2171-2172
 
+  if (VD->isPlaceholderVar())
+    return;
+
----------------
cor3ntin wrote:
> aaron.ballman wrote:
> > What's the rationale here? I know the standard has a recommended practice, but I don't see why that should apply to the case where the user assigns a value to the placeholder but never references it otherwise.
> The intent is that this behave like `[[maybe_unused]]`
Ah, thank you! I think we should make a slightly wider change here then. Let's move `VD->hasAttr<UnusedAttr>()` out of the first if statement and into this one, then add a comment about the relationship between the attribute and this feautre. WDYT?


================
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}}
----------------
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.


================
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}}
----------------
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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153536