Don't warn about missing declarations for partial template specializations

[aaronpuchert]

Summary

Just like templates, they are excepted from the ODR rule.

[aaronpuchert]

Gentle ping.

I'm open to suggestions to simplify the condition, but I think templates, partial specializations and instantiations are three different things that we all need to exclude here.

[aaron.ballman]

I think this change is reasonable, but I'd like @rsmith to agree before you commit.

[aaronpuchert]

@rsmith, could you please have a look?

[aaronpuchert]

Ping.

[aaronpuchert]

Another ping.

[aaron.ballman]

I think the pings have gone on long enough -- if there's an issue with the patch, we can address it post-commit.

[rsmith]

Sorry I missed this before now.

[aaronpuchert]

Thanks for the reviews! Do you think it makes sense to bring this to Clang 10?

[aaron.ballman]

In D68923#1853302, @aaronpuchert wrote:

Thanks for the reviews! Do you think it makes sense to bring this to Clang 10?

I think it's a simple enough fix that it may be worth it, but it isn't fixing a regression in behavior so it's not critical. If it helps you out to move it onto the 10 branch, I think it's fine.

[aaronpuchert]

In D68923#1853303, @aaron.ballman wrote:

In D68923#1853302, @aaronpuchert wrote:

Thanks for the reviews! Do you think it makes sense to bring this to Clang 10?

I think it's a simple enough fix that it may be worth it, but it isn't fixing a regression in behavior so it's not critical.

Exactly, it would just be a bug fix.

If it helps you out to move it onto the 10 branch, I think it's fine.

I might be porting this back anyways on downstream, but I don't want to be selfish.

[aaronpuchert]

In D68923#1853527, @aaronpuchert wrote:

In D68923#1853303, @aaron.ballman wrote:

I think it's a simple enough fix that it may be worth it, but it isn't fixing a regression in behavior so it's not critical.

Exactly, it would just be a bug fix.

Actually it is a regression, but one that we already had in Clang 9. Try this code:

template <typename T>
constexpr bool X = true;

template <typename T>
constexpr bool X<T*> = false;

Clang 9 emits the following false positive warning, which Clang 8 doesn't emit:

<source>:5:16: warning: no previous extern declaration for non-static variable 'X<T *>' [-Wmissing-variable-declarations]
constexpr bool X<T*> = false;
               ^
<source>:5:11: note: declare 'static' if the variable is not intended to be used outside of this translation unit
constexpr bool X<T*> = false;
          ^

[aaron.ballman]

In D68923#1857046, @aaronpuchert wrote:

In D68923#1853527, @aaronpuchert wrote:

In D68923#1853303, @aaron.ballman wrote:

I think it's a simple enough fix that it may be worth it, but it isn't fixing a regression in behavior so it's not critical.

Exactly, it would just be a bug fix.

Actually it is a regression, but one that we already had in Clang 9. Try this code:

template <typename T>
constexpr bool X = true;

template <typename T>
constexpr bool X<T*> = false;

Clang 9 emits the following false positive warning, which Clang 8 doesn't emit:

<source>:5:16: warning: no previous extern declaration for non-static variable 'X<T *>' [-Wmissing-variable-declarations]
constexpr bool X<T*> = false;
               ^
<source>:5:11: note: declare 'static' if the variable is not intended to be used outside of this translation unit
constexpr bool X<T*> = false;
          ^

Feel free to suggest to Hans to add this to the 10.0 release.

[aaronpuchert]

@hans, could you cherry-pick this on the version 10 branch? As I wrote in D68923#1857046, this is a regression from Clang 8.

[hans]

In D68923#1857307, @aaronpuchert wrote:

@hans, could you cherry-pick this on the version 10 branch? As I wrote in D68923#1857046, this is a regression from Clang 8.

Sounds good to me, cherry-picked as fd271fd64a284e9182c8afd8eb8084d8d43df587

clang/lib/Sema/SemaDecl.cpp

void Sema::CheckCompleteVariableDeclaration(VarDecl *var) {
   // prior declaration.  We only want to do this for global
   // declarations, but we also specifically need to avoid doing it for
   // class members because the linkage of an anonymous class can
   // change if it's later given a typedef name.
   if (var->isThisDeclarationADefinition() &&
       var->getDeclContext()->getRedeclContext()->isFileContext() &&
       var->isExternallyVisible() && var->hasLinkage() &&
       !var->isInline() && !var->getDescribedVarTemplate() &&
+      !isa<VarTemplatePartialSpecializationDecl>(var) &&
       !isTemplateInstantiation(var->getTemplateSpecializationKind()) &&
       !getDiagnostics().isIgnored(diag::warn_missing_variable_declarations,
                                   var->getLocation())) {
     // Find a previous declaration that's not a definition.
     VarDecl *prev = var->getPreviousDecl();
     while (prev && prev->isThisDeclarationADefinition())
       prev = prev->getPreviousDecl();
 

clang/test/SemaCXX/warn-missing-variable-declarations.cpp

 // expected-note@-1{{declare 'static' if the variable is not intended to be used outside of this translation unit}}
 extern constexpr int extern_constexpr_var = 0; // expected-warning {{no previous extern declaration}}
 // expected-note@-1{{declare 'static' if the variable is not intended to be used outside of this translation unit}}
 
 template<typename> int var_template = 0;
 template<typename> constexpr int const_var_template = 0;
 template<typename> static int static_var_template = 0;
 
+template<typename T> int var_template<T*>;
+
 template int var_template<int[1]>;
 int use_var_template() { return var_template<int[2]>; }
 template int var_template<int[3]>;
 extern template int var_template<int[4]>;
 template<> int var_template<int[5]>; // expected-warning {{no previous extern declaration}}
 // expected-note@-1{{declare 'static' if the variable is not intended to be used outside of this translation unit}}
 
 // FIXME: We give this specialization internal linkage rather than inheriting
 // the linkage from the template! We should not warn here.
 template<> int static_var_template<int[5]>; // expected-warning {{no previous extern declaration}}
 // expected-note@-1{{declare 'static' if the variable is not intended to be used outside of this translation unit}}