[PATCH] D68923: Don't warn about missing declarations for partial template specializations

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

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 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68923



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68923: Don't warn about missing declarations for partial template specializations

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a subscriber: hans.
aaronpuchert added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68923



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68923: Don't warn about missing declarations for partial template specializations

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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 
>   constexpr bool X = true;
>  
>   template 
>   constexpr bool X = false;
>
>
> Clang 9 emits the following false positive warning, which Clang 8 doesn't 
> emit:
>
>   :5:16: warning: no previous extern declaration for non-static 
> variable 'X' [-Wmissing-variable-declarations]
>   constexpr bool X = false;
>  ^
>   :5:11: note: declare 'static' if the variable is not intended to be 
> used outside of this translation unit
>   constexpr bool X = false;
> ^
>


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68923



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68923: Don't warn about missing declarations for partial template specializations

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

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 
  constexpr bool X = true;
  
  template 
  constexpr bool X = false;

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

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68923



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68923: Don't warn about missing declarations for partial template specializations

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68923



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68923: Don't warn about missing declarations for partial template specializations

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68923



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68923: Don't warn about missing declarations for partial template specializations

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68923



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68923: Don't warn about missing declarations for partial template specializations

[Aaron Puchert via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG27684ae66d55: Don't warn about missing declarations for 
partial template specializations (authored by aaronpuchert).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68923

Files:
  clang/lib/Sema/SemaDecl.cpp
  clang/test/SemaCXX/warn-missing-variable-declarations.cpp


Index: clang/test/SemaCXX/warn-missing-variable-declarations.cpp
===
--- clang/test/SemaCXX/warn-missing-variable-declarations.cpp
+++ clang/test/SemaCXX/warn-missing-variable-declarations.cpp
@@ -70,6 +70,8 @@
 template constexpr int const_var_template = 0;
 template static int static_var_template = 0;
 
+template int var_template;
+
 template int var_template;
 int use_var_template() { return var_template; }
 template int var_template;
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -12523,6 +12523,7 @@
   var->getDeclContext()->getRedeclContext()->isFileContext() &&
   var->isExternallyVisible() && var->hasLinkage() &&
   !var->isInline() && !var->getDescribedVarTemplate() &&
+  !isa(var) &&
   !isTemplateInstantiation(var->getTemplateSpecializationKind()) &&
   !getDiagnostics().isIgnored(diag::warn_missing_variable_declarations,
   var->getLocation())) {


Index: clang/test/SemaCXX/warn-missing-variable-declarations.cpp
===
--- clang/test/SemaCXX/warn-missing-variable-declarations.cpp
+++ clang/test/SemaCXX/warn-missing-variable-declarations.cpp
@@ -70,6 +70,8 @@
 template constexpr int const_var_template = 0;
 template static int static_var_template = 0;
 
+template int var_template;
+
 template int var_template;
 int use_var_template() { return var_template; }
 template int var_template;
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -12523,6 +12523,7 @@
   var->getDeclContext()->getRedeclContext()->isFileContext() &&
   var->isExternallyVisible() && var->hasLinkage() &&
   !var->isInline() && !var->getDescribedVarTemplate() &&
+  !isa(var) &&
   !isTemplateInstantiation(var->getTemplateSpecializationKind()) &&
   !getDiagnostics().isIgnored(diag::warn_missing_variable_declarations,
   var->getLocation())) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68923: Don't warn about missing declarations for partial template specializations

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.

Sorry I missed this before now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68923



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68923: Don't warn about missing declarations for partial template specializations

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68923



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68923: Don't warn about missing declarations for partial template specializations

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

Another ping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68923



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68923: Don't warn about missing declarations for partial template specializations

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

Ping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68923



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68923: Don't warn about missing declarations for partial template specializations

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

@rsmith, could you please have a look?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68923



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68923: Don't warn about missing declarations for partial template specializations

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68923



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68923: Don't warn about missing declarations for partial template specializations

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68923



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68923: Don't warn about missing declarations for partial template specializations

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert created this revision.
aaronpuchert added reviewers: aaron.ballman, rsmith.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D68923

Files:
  clang/lib/Sema/SemaDecl.cpp
  clang/test/SemaCXX/warn-missing-variable-declarations.cpp


Index: clang/test/SemaCXX/warn-missing-variable-declarations.cpp
===
--- clang/test/SemaCXX/warn-missing-variable-declarations.cpp
+++ clang/test/SemaCXX/warn-missing-variable-declarations.cpp
@@ -70,6 +70,8 @@
 template constexpr int const_var_template = 0;
 template static int static_var_template = 0;
 
+template int var_template;
+
 template int var_template;
 int use_var_template() { return var_template; }
 template int var_template;
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -12345,6 +12345,7 @@
   var->getDeclContext()->getRedeclContext()->isFileContext() &&
   var->isExternallyVisible() && var->hasLinkage() &&
   !var->isInline() && !var->getDescribedVarTemplate() &&
+  !isa(var) &&
   !isTemplateInstantiation(var->getTemplateSpecializationKind()) &&
   !getDiagnostics().isIgnored(diag::warn_missing_variable_declarations,
   var->getLocation())) {


Index: clang/test/SemaCXX/warn-missing-variable-declarations.cpp
===
--- clang/test/SemaCXX/warn-missing-variable-declarations.cpp
+++ clang/test/SemaCXX/warn-missing-variable-declarations.cpp
@@ -70,6 +70,8 @@
 template constexpr int const_var_template = 0;
 template static int static_var_template = 0;
 
+template int var_template;
+
 template int var_template;
 int use_var_template() { return var_template; }
 template int var_template;
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -12345,6 +12345,7 @@
   var->getDeclContext()->getRedeclContext()->isFileContext() &&
   var->isExternallyVisible() && var->hasLinkage() &&
   !var->isInline() && !var->getDescribedVarTemplate() &&
+  !isa(var) &&
   !isTemplateInstantiation(var->getTemplateSpecializationKind()) &&
   !getDiagnostics().isIgnored(diag::warn_missing_variable_declarations,
   var->getLocation())) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits