This is an archive of the discontinued LLVM Phabricator instance.

[analyzer][NFC] Remove useless class BuiltinBug
ClosedPublic

Authored by donat.nagy on Aug 25 2023, 8:39 AM.

Details

Summary

...because it provides no useful functionality compared to its base class BugType.

A long time ago there were substantial differences between BugType and BuiltinBug, but they were eliminated by commit 1bd58233 in 2009 (!). Since then the only functionality provided by BuiltinBug was that it specified categories::LogicError as the bug category and it stored an extra data member desc.

This commit sets categories::LogicError as the default value of the third argument (bug category) in the constructors of BugType and replaces use of the desc field with simpler logic.

Note that BugType has a data member Description and a non-virtual method BugType::getDescription() which queries it; these are distinct from the member desc of BuiltinBug and the shadowing method BuiltinBug::getDescription() which queries it. This confusing name collision was a major motivation for the elimination of BuiltinBug.

As this commit touches many files, I avoided functional changes and left behind several FIXME notes on messages that should be improved later.

Diff Detail

Event Timeline

donat.nagy created this revision.Aug 25 2023, 8:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 25 2023, 8:39 AM
donat.nagy requested review of this revision.Aug 25 2023, 8:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 25 2023, 8:39 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
donat.nagy added inline comments.Aug 25 2023, 8:46 AM
clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
35–36

This inconsistency is the "original target" of this commit. I wanted to unify the two types of bug report creation ("regular" and tainted) and as I examined the situation I noticed that BuiltinBug isa confusing mess and it's always easy to replace it with BugType.

xazax.hun added inline comments.Aug 25 2023, 9:02 AM
clang/lib/StaticAnalyzer/Checkers/BoolAssignmentChecker.cpp
27

In case you are down to some additional cleanup efforts, we no longer need lazy initialization for BugTypes. This is an old pattern, we used to need it due to some initialization order problems, but those have been worked around in engine. See the FuchsiaHandleChecker as an example how we do it in newer checks: https://github.com/llvm/llvm-project/blob/ea82a822d990824c58c6fa4b503ca84c4870c940/clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp#L190

Feel free to ignore this or do it in a follow-up PR.

donat.nagy marked an inline comment as done.Aug 28 2023, 1:25 AM
donat.nagy added inline comments.
clang/lib/StaticAnalyzer/Checkers/BoolAssignmentChecker.cpp
27

Great news! I'll keep it in mind and I'll try to do this clean-up if I have enough free time.

steakhal accepted this revision.Aug 28 2023, 2:57 AM

I haven't checked the details, but it makes sense.
It's reassuring that all the tests pass :D, which is good enough for me.

Thanks for the cleanup!

This revision is now accepted and ready to land.Aug 28 2023, 2:57 AM
This revision was automatically updated to reflect the committed changes.
donat.nagy marked an inline comment as done.