Page MenuHomePhabricator

Bug 49633 - Added warning for static inline global and namespaced declarations
Needs ReviewPublic

Authored by serberoth on May 17 2021, 8:12 PM.

Details

Reviewers
erik.pilkington
rsmith
TH3CHARLie
Group Reviewers
Restricted Project
Summary

Bug 49633 - Added a warning for global and namespace declared variables that are declared as both static and inline.

Diff Detail

Event Timeline

serberoth requested review of this revision.May 17 2021, 8:12 PM
serberoth created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2021, 8:12 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
erichkeane added inline comments.
clang/lib/Sema/SemaDecl.cpp
7127

First, what about this is C++17 specific? The inline and static relationship is older than C++17, right?

Also, are you sure that the warning/stuff in the 'else' isn't still valuable?

serberoth added inline comments.May 21 2021, 1:40 PM
clang/lib/Sema/SemaDecl.cpp
7127

Perhaps I misunderstood something in the bug write-up, the component for the ticket is C++17. Also there is the warning that inline variables are a C++17 extension that appears when you use the inline keyword on a variable (regardless of the appearance of the static keyword). I suppose that does not necessarily mean we cannot simply show both warnings, and maybe that is the right thing to do. I felt that was not really necessary because of the other warning.

As for the other warnings in the else the one is the warning that I mentioned (which only applies when the C++17 standard is not applied, and the other is a C++14 compatibility warning which states:
"inline variables are incompatible with C++ standards before C++17"

You can see the messages for those diagnostic message here:
https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/DiagnosticSemaKinds.td#L1467

Please let me know if I am still missing something with how this diagnostic was supposed to apply and where. Thank you.

Should we also warn about inline variables in anonymous namespaces?

clang/lib/Sema/SemaDecl.cpp
7127

This diagnostic should be independent of the ext / compat diagnostic below. This patch currently removes the -Wc++14-compat warning for static inline int n;, which would be a behavior regression for that warning.

Moving this check inside the else block a few lines below, and removing the check for CPlusPlus17, would seem reasonable. We accept inline variables in earlier language modes as an extension, and we should still warn in that case.

serberoth updated this revision to Diff 348156.May 26 2021, 9:52 PM
serberoth retitled this revision from Bug 49633 - Added warning for static inline global and namespaced declarations for c++17+ to Bug 49633 - Added warning for static inline global and namespaced declarations.
serberoth edited the summary of this revision. (Show Details)

Updates to testing to ensure warning occurs for variables declared in anonymous namespace.
Updates per rsmith to ensure that ext compat warning still occurs fixing regressive behaviour.