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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added subscribers: cjdb, aaron.ballman.
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.
 
----------------
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?


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


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


================
Comment at: clang/include/clang/Sema/Lookup.h:121
+    /// Name lookup results in an ambiguity because multiple placeholder
+    /// variables were found in the same scope
+    /// @code
----------------



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


================
Comment at: clang/lib/Sema/SemaDecl.cpp:7481
+    if (!Previous.empty()) {
+      auto Decl = *Previous.begin();
+      const bool sameDC = Decl->getDeclContext()->getRedeclContext()->Equals(
----------------
Please spell out the type here.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:7482
+      auto Decl = *Previous.begin();
+      const bool sameDC = Decl->getDeclContext()->getRedeclContext()->Equals(
+          DC->getRedeclContext());
----------------



================
Comment at: clang/lib/Sema/SemaDecl.cpp:7484-7486
+      if (sameDC && isDeclInScope(Decl, CurContext, S, false)) {
+        DiagPlaceholderVariableDefinition(D.getIdentifierLoc());
+      }
----------------



================
Comment at: clang/lib/Sema/SemaDecl.cpp:8331
 
+  // Never warn about shadowing placeholder variable
+  if (ShadowedDecl->isPlaceholderVar())
----------------



================
Comment at: clang/lib/Sema/SemaDecl.cpp:18217
 
+  NewFD->setIsPlaceholderVar(LangOpts.CPlusPlus && II && II->isPlaceholder());
   if (PrevDecl && !isa<TagDecl>(PrevDecl)) {
----------------
shafik wrote:
> cor3ntin wrote:
> > shafik wrote:
> > > Why can't we fold this into `FieldDecl::Create`? This comment applies in some other places as well.
> > By adding a parameter to FieldDecl::Create? We could, I'm not sure it would necessarily be cleaner. Let me know what you prefer.
> It seems like having the code consolidated make for better maintainability and figuring this out for future folks but I am willing to be convinced otherwise.
We need to support partial construction because of AST deserialization, but it seems to me that we don't need to add a new parameter to `FieldDecl::Create()` anyway; can't we have the `FieldDecl` constructor look at the given `IdentifierInfo *` in that case, and if it's nonnull, set the placeholder from that?


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:886
     DeclarationNameInfo NameInfo(B.Name, B.NameLoc);
+    IdentifierInfo *VarName = B.Name;
     LookupResult Previous(*this, NameInfo, LookupOrdinaryName,
----------------
`assert(VarName && "Cannot have an unnamed binding declaration");` ?


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:917
+      if (IsPlaceholder) {
+        const bool sameDC = (Previous.end() - 1)
+                                ->getDeclContext()
----------------



================
Comment at: clang/lib/Sema/SemaLambda.cpp:1151-1153
+    if (Var && Var->isInitCapture() && C->Id->isPlaceholder()) {
+      Var->setIsPlaceholderVar(true);
+    }
----------------



================
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}}
----------------
I don't understand what this diagnostic is trying to tell me, can you explain a bit more?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153536