[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/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.
 
----------------
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.


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


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


================
Comment at: clang/lib/Sema/SemaDecl.cpp:2171-2172
 
+  if (VD->isPlaceholderVar())
+    return;
+
----------------
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]]`


================
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:
> 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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153536