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

[Joe Ranieri via Phabricator via cfe-commits]

jranieri-grammatech updated this revision to Diff 295563.
jranieri-grammatech added a comment.
Herald added a project: clang.

In D54222#2295374 <https://reviews.llvm.org/D54222#2295374>, @JonasToth wrote:

> 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.

I ran it on our large internal codebase. It generated one unique warning (2 total) with the filter on `/include/`. It was a false positive but at least it was actually complaining about something plugin-related. When loosening it up to any included file, that grows to five unique warnings (231 total) and all of the additional warnings were false positives.

> 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 think the fact that it's triggered in header files makes it inherently noisier than other checkers. The warnings in LLVM I pasted above were only the unique instances -- there were a total of 323 warnings issued for the first set and a total of 353 warnings issued when relaxing the restrictions.

> 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".

I've updated the patch and moved it into `bugprone-`. I think it probably needs a better name than `bugprone-problematic-statics`, now that it's expected to be used outside of the LLVM context.

> 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?).

It can't be decided completely. I think that this checker would be more useful with a configuration option to specify a regex for "plugin API headers", but I don't have the time to implement it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54222

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/ProblematicStaticsCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/ProblematicStaticsCheck.h
  clang-tools-extra/docs/clang-tidy/checks/bugprone-problematic-statics.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-problematic-statics/bugprone-problematic-statics.h
  clang-tools-extra/test/clang-tidy/checkers/bugprone-problematic-statics.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D54222.295563.patch
Type: text/x-patch
Size: 7346 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20201001/c1df39e0/attachment-0001.bin>