This is an archive of the discontinued LLVM Phabricator instance.

Don't warn about missing declarations for partial template specializations
ClosedPublic

Authored by aaronpuchert on Oct 12 2019, 5:04 PM.

Diff Detail

Event Timeline

aaronpuchert created this revision.Oct 12 2019, 5:04 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 12 2019, 5:04 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

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.

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

@rsmith, could you please have a look?

aaron.ballman accepted this revision.Jan 31 2020, 6:53 AM

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

This revision is now accepted and ready to land.Jan 31 2020, 6:53 AM
rsmith accepted this revision.Jan 31 2020, 10:24 AM

Sorry I missed this before now.

This revision was automatically updated to reflect the committed changes.

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

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.

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.

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;
          ^

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.

@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 added a comment.Feb 5 2020, 6:26 AM

@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