[PATCH] D54222: [clang-tidy] Add a check to detect returning static locals in public headers

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

In D54222#2294123 <https://reviews.llvm.org/D54222#2294123>, @jranieri-grammatech wrote:

> In D54222#1290789 <https://reviews.llvm.org/D54222#1290789>, @JonasToth wrote:
>
>> I agree with @Eugene.Zelenko that this check should rather live in `bugprone-` as the issue its diagnosing is not LLVM specific. It is ok to add an alias into the llvm module.
>
> Here are the warnings it issues with the patch's matcher:
>
>   clang/include/clang/StaticAnalyzer/Core/CheckerManager.h:625:43: warning: address of static local variable 'tag' may not be identical across library boundaries [llvm-problematic-statics]
>   clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:219:1: warning: address of static local variable 'Index' may not be identical across library boundaries [llvm-problematic-statics]
>   clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConstraintManager.h:25:1: warning: address of static local variable 'Index' may not be identical across library boundaries [llvm-problematic-statics]
>
> And here are the additional warnings it issues when relaxing the matcher (to be `unless(isExpansionInMainFile())`):
>
>   clang/lib/StaticAnalyzer/Checkers/Iterator.h:130:47: warning: address of static local variable 'Index' may not be identical across library boundaries [llvm-problematic-statics]
>   clang/lib/StaticAnalyzer/Checkers/Iterator.h:136:47: warning: address of static local variable 'Index' may not be identical across library boundaries [llvm-problematic-statics]
>   clang/lib/StaticAnalyzer/Checkers/Iterator.h:142:47: warning: address of static local variable 'Index' may not be identical across library boundaries [llvm-problematic-statics]
>   clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPITypes.h:60:5: warning: address of static local variable 'index' may not be identical across library boundaries [llvm-problematic-statics]
>   llvm/utils/unittest/googletest/src/gtest.cc:3989:3: warning: address of static local variable 'instance' may not be identical across library boundaries [llvm-problematic-statics]
>
> And also this one, which was hidden by default but made running tidy noisier by always saying there was a hidden warning:
>
>   /usr/bin/../include/c++/v1/functional:1960:9: warning: address of static local variable '__policy_' may not be identical across library boundaries [llvm-problematic-statics]
>
> All of the first set of warnings might be actual problems, but the second set definitely aren't problems because they aren't going to be used across shared library boundaries. By making this be a LLVM-specific check, we can avoid the noise of these false positives. Also, I looked through the C++ Core Guidelines for any rules that might address this but didn't see anything relevant.
>
> Thoughts?

Do you have data for other projects? As this is not a very common thing and probably different for code-bases with plugins and so on, the "chattiness" of the check would be interesting to know.
If the check is usually quiet, then i would think its ok to have it as a general check together with proper documentation explaining why these statics can be a problem.

I would still like to have it in `bugprone-`, because this is a real problem that can arise and the check is more likely to be ignored if considered to be "just for llvm".
The decision for true vs false positive is not possible to decide within clang-tidy, is it? I think this should then be left to the developer (as it is probably not that much?).


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54222