[PATCH] D139113: [clang-tidy] Fix a couple additional cases in misc-use-anonymous-namespace

Mon Dec 12 06:39:35 PST 2022

carlosgalvezp added inline comments.

================
Comment at: clang-tools-extra/test/clang-tidy/checkers/misc/use-anonymous-namespace.cpp:46-48
+// OK
+static const int v8{123};
+static constexpr int v9{123};
----------------
vingeldal wrote:
> Is it really the best behavior to allow these? If I got the rationale right we don't warn about this because const and constexpr have implicit internal linkage anyway, so static doesn't make a difference, right?
> Reading the documentation for this check I gather static would probably have been deprecated if it wasn't for the fact that deprecation would have broken compatibility with C. So, if we drop the static keyword here we still get the behavior we want, without a confusing keyword we would rather get rid of if we could, while keeping compatibility with C.
> 
> I'm thinking it could be better to just discourage from using static for cases like this.
Thanks for the review!

Yes, that example is "bad" code, but this particular check would currently ask users to move the variables to the anonymous namespace - which is also incorrect.

My plan is to add one more separate checks in the "readability" section that warn about redundant usages of 'static' (like this one), and even supports automatic fixes (which aren't possible in this check).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139113