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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
