[PATCH] D102663: Bug 49633 - Added warning for static inline global and namespaced declarations for c++17+

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

rsmith added a comment.

Should we also warn about `inline` variables in anonymous namespaces?



================
Comment at: clang/lib/Sema/SemaDecl.cpp:7127
         << FixItHint::CreateRemoval(D.getDeclSpec().getInlineSpecLoc());
+    } else if (SC == SC_Static && DC->isFileContext() &&
+               getLangOpts().CPlusPlus17) {
----------------
serberoth wrote:
> erichkeane wrote:
> > 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?
> 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102663