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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I've not spotted anything major, but I would like some additional test coverage. Btw, you need to add this to the table in https://github.com/llvm/llvm-project/blob/189033e6bede96de0d74e61715fcd1b48d95e247/clang/docs/LanguageExtensions.rst?plain=1#L1429 because this is an extension we intentionally support in older language modes (we want to let folks like libc++ rely on this behavior in Clang).



================
Comment at: clang/lib/AST/Decl.cpp:1115-1122
+  if (const VarDecl *VD = dyn_cast<VarDecl>(this)) {
+    if (isa<ParmVarDecl>(VD))
+      return false;
+    if (VD->isInitCapture())
+      return true;
+    return VD->getStorageDuration() == StorageDuration::SD_Automatic;
+  }
----------------



================
Comment at: clang/lib/Sema/SemaDecl.cpp:1979
 
-static bool ShouldDiagnoseUnusedDecl(const NamedDecl *D) {
+static bool ShouldDiagnoseUnusedDecl(const Sema &SemaRef, const NamedDecl *D) {
   if (D->isInvalidDecl())
----------------
This could take `const LangOptions &` instead of `const Sema &` as we don't actually use `Sema` here.


================
Comment at: clang/test/SemaCXX/cxx2c-placeholder-vars.cpp:3
+
+void static_var() {
+    static int _; // expected-note {{previous definition is here}} \
----------------
Can you also add tests for global namespace and anonymous namespaces?

Can you add coverage for:
```
int _ = 12;
auto lam = [_ = 0]{}; // gets a placeholder extension warning (I don't think we issue a shadow warning currently, but probably should?)
```
and
```
void func() {
  // Is a placeholder variable and so it does not get a -Wunused-but-set warning despite
  int _ = 12;
  int x = 12; // Gets -Wunused-but-set warning
}
```



================
Comment at: clang/test/SemaCXX/cxx2c-placeholder-vars.cpp:19
+    int arr[4] = {0, 1, 2, 3};
+    auto [_, _, _, _] = arr; // expected-warning 3{{placeholder variables are a C++2c extension}} \
+                             // expected-note 4{{placeholder declared here}}
----------------
I think we want additional comments explaining why there's only three instances of this warning instead of four because this situation is kind of weird. The first declaration *is* a placeholder, so we suppress unused variable warnings for it. However, because a single variable named `_` is valid code in earlier language modes, it's not really an *extension* until we introduce the second variable named `_` in the same scope in terms of conformance. It doesn't help users to issue an extension warning on the first declaration because the diagnostic behavior change is more likely appreciated than a surprising change.

I think this information should be documented somewhere (perhaps just in the release notes with an example since we don't have much to document about the extension itself).


================
Comment at: clang/test/SemaCXX/cxx2c-placeholder-vars.cpp:52
+void _() {} // expected-error {{redefinition of '_'}}
+void _(int){};
+}
----------------



================
Comment at: clang/test/SemaCXX/cxx2c-placeholder-vars.cpp:63-70
+struct Members {
+    int _; // expected-note {{placeholder declared here}}
+    int _; // expected-warning{{placeholder variables are a C++2c extension}} \
+           // expected-note {{placeholder declared here}}
+    void f() {
+        _++; // expected-error {{ambiguous reference to placeholder '_', which is defined multiple times}}
+    }
----------------
Can you add a test for this case:
```
struct WhoWritesCodeLikeThis {
  int _;
  void f() {
    int _;
    _ = 12; // Not ambiguous reference to placeholder, accesses the local variable which shadows the field
    (void)({ int _ = 12; _;}); // Also not an ambiguous reference to a placeholder, accesses the GNU statement expression variable which shadows the local variable
  }
  // None of the _ declarations should get the extension warning
};
```
and for:
```
typedef int _;
typedef int _; // Not a placeholder, but also not a redefinition, so this_is_fine.gif
```
and for:
```
struct S {
  int _, _;
};
constexpr int oh_no = __builtin_offsetof(S, _); // Ambiguous reference error
int S::* ref = &S::_; // Ambiguous reference error
```
and for:
```
struct S {
  int _, _;
  void func() __attribute__((diagnose_if(_ != 0, "oh no!", "warning")));
};
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153536