Is getCanonicalDecl() working correctly here?

Beginners
1

Hey,

I’ve been looking into a difference in compilation warnings between MSVC and clang with the following modified test code from clang/test/CXX/basic/basic.def/p2.cpp:

      1 // RUN: %clang_cc1 -std=c++1z -verify %s -Wdeprecated
      2 
      3 namespace {
      4   struct A {
      5     static constexpr int n = 0;
      6   };
      7   const int A::n; // expected-warning {{deprecated}}
      8 
      9   struct B {
     10     static constexpr int m = 0;
     11   };
     12   constexpr int B::m; // expected-warning {{deprecated}}
     13 }

Compiler Explorer shows that MSVC emits warnings for both lines 7 and 12, but clang only emits a warning for the first line. Through debugging clang I found the difference is in the return value for New->isThisDeclarationADefinition() on line 4540 of clang/lib/Sema/SemaDecl.cpp:

   4538   // C++ doesn't have tentative definitions, so go right ahead and check here.
   4539   if (getLangOpts().CPlusPlus &&
   4540       New->isThisDeclarationADefinition() == VarDecl::Definition) {
   4541     if (Old->isStaticDataMember() && Old->getCanonicalDecl()->isInline() &&
   4542         Old->getCanonicalDecl()->isConstexpr()) {
   4543       // This definition won't be a definition any more once it's been merged.
   4544       Diag(New->getLocation(),
   4545            diag::warn_deprecated_redundant_constexpr_static_def);
   4546     } else if (VarDecl *Def = Old->getDefinition()) {
   4547       if (checkVarDeclRedefinition(Def, New))
   4548         return;
   4549     }
   4550   }

With A::n returning VarDecl::Definition and B::m returning VarDecl::DeclarationOnly. The relevant code inside clang/lib/AST/Decl.cpp is below, I’ve added the comment on lines 2184-2185 in my local copy:

   2164 VarDecl::DefinitionKind
   2165 VarDecl::isThisDeclarationADefinition(ASTContext &C) const {
   2166   if (isThisDeclarationADemotedDefinition())
   2167     return DeclarationOnly;
   2168 
   2169   // C++ [basic.def]p2:
   2170   //   A declaration is a definition unless [...] it contains the 'extern'
   2171   //   specifier or a linkage-specification and neither an initializer [...],
   2172   //   it declares a non-inline static data member in a class declaration [...],
   2173   //   it declares a static data member outside a class definition and the variable
   2174   //   was defined within the class with the constexpr specifier [...],
   2175   // C++1y [temp.expl.spec]p15:
   2176   //   An explicit specialization of a static data member or an explicit
   2177   //   specialization of a static data member template is a definition if the
   2178   //   declaration includes an initializer; otherwise, it is a declaration.
   2179   //
   2180   // FIXME: How do you declare (but not define) a partial specialization of
   2181   // a static data member template outside the containing class?
   2182   if (isStaticDataMember()) {
   2183     if (isOutOfLine() &&
   2184         // Both false in 'const' case, so we return "Definition", both true in 'constexpr'
   2185         // case, so we return "DeclarationOnly"
   2186         !(getCanonicalDecl()->isInline() &&
   2187           getCanonicalDecl()->isConstexpr()) &&
   2188         (hasInit() ||
   2189          // If the first declaration is out-of-line, this may be an
   2190          // instantiation of an out-of-line partial specialization of a variable
   2191          // template for which we have not yet instantiated the initializer.
   2192          (getFirstDecl()->isOutOfLine()
   2193               ? getTemplateSpecializationKind() == TSK_Undeclared
   2194               : getTemplateSpecializationKind() !=
   2195                     TSK_ExplicitSpecialization) ||
   2196          isa<VarTemplatePartialSpecializationDecl>(this)))
   2197       return Definition;
   2198     if (!isOutOfLine() && isInline())
   2199       return Definition;
   2200     return DeclarationOnly;
   2201   }

In the A::n case the getCanonicalDecl() calls on lines 2186 and 2187 return the declaration on line 7 of the example code, which is neither inline nor a constexpr, so the if expression started on line 2183 ultimately evaluates as true. In the B::m input case getCanonicalDecl() returns the definition on line 10 of the example code, which is both inline and a constexpr, so the if condition is false.

This is where I’m confused. My reading of the standard comment included above is that the intention of this code is that getCanonicalDecl() in the A::n case should return the first declaration of A::n, which is on line 5 of the example input. Therefore the declaration on line 7 is not a definition, because it “declares a static member outside a class definition and the variable was defined within the class with the constexpr specifier.”

If that’s so, then, both getCanonicalDecl() and the calling code in SemaDecl.cpp are erroneous. But the intention of the warning, IIUC, is to point out redundant definitions that in C++17 are back to declarations. But I also don’t understand how const int A::n would have been thought of as a definition at any C++ standard.

I’d really appreciate any clarification that folks can provide.

Thanks!

2

In my testing, both for A::n and B::m, getCanonicalDecl() returns the “new” decl, i.e. the one on line 7 and 12, respectively.

I tested by inserting “printfs” like this:

$ git diff
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index df3e8804c7d5..aa464ffa67da 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -4536,6 +4536,9 @@ void Sema::MergeVarDecl(VarDecl *New, LookupResult &Previous) {
   }
 
   // C++ doesn't have tentative definitions, so go right ahead and check here.
+  llvm::errs() << "New:\n"; New->dump();
+  llvm::errs() << "New->getCanonicalDecl():\n"; New->getCanonicalDecl()->dump();
+
   if (getLangOpts().CPlusPlus &&
       New->isThisDeclarationADefinition() == VarDecl::Definition) {
     if (Old->isStaticDataMember() && Old->getCanonicalDecl()->isInline() &&

As you say, normally, I think getCanonicalDecl() for a variable would return the first declaration, but at the point when we’re looking (Sema::MergeVarDecl) the new declaration is still being processed and hasn’t been “merged” with the previous one yet.

I think the issue here is that in the B::m case, the second declaration isn’t considered a definition according to the comment:

// C++ [basic.def]p2:
//   A declaration is a definition unless
[...]
//   it declares a static data member outside a class definition and the variable
//   was defined within the class with the constexpr specifier [...],

That seems to come from here: [basic.def]

I think the logic in Sema::MergeVarDecl() needs to be expanded to take this into account.

3

Thanks so much for the help! I found the example of using llvm::errs() for logging particularly useful. I have a patch out today to fix the logic around this warning.