[PATCH] D158855: [analyzer][NFC] Remove useless class BuiltinBug

[Donát Nagy via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
donat.nagy marked an inline comment as done.
Closed by commit rG8a5cfdf7851d: [analyzer][NFC] Remove useless class 
BuiltinBug (authored by donat.nagy).

Changed prior to commit:
  https://reviews.llvm.org/D158855?vs=553498&id=553909#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158855

Files:
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugType.h
  clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
  clang/lib/StaticAnalyzer/Checkers/BoolAssignmentChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/CastSizeChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/DebugCheckers.cpp
  clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/FixedAddressChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ObjCAtSyncChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/PointerSubChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ReturnPointerRangeChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ReturnUndefChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/TestAfterDivZeroChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/UndefBranchChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/UndefinedArraySubscriptChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp
  
clang/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/VforkChecker.cpp
  clang/lib/StaticAnalyzer/Core/BugReporter.cpp
  clang/unittests/StaticAnalyzer/CallEventTest.cpp
  clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
  clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp

Index: clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
===
--- clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
+++ clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
@@ -89,8 +89,8 @@
 
 class CheckerRegistrationOrderPrinter
 : public Checker> {
-  std::unique_ptr BT =
-  std::make_unique(this, "Registration order");
+  std::unique_ptr BT =
+  std::make_unique(this, "Registration order");
 
 public:
   void checkPreStmt(const DeclStmt *DS, CheckerContext &C) const {
@@ -125,8 +125,7 @@
 
 #define UNITTEST_CHECKER(CHECKER_NAME, DIAG_MSG)   \
   class CHECKER_NAME : public Checker> {  \
-std::unique_ptr BT =   \
-std::make_unique(this, DIAG_MSG);  \
+std::unique_ptr BT = std::make_unique(this, DIAG_MSG);   \
\
   public:  \
 void checkPreStmt(const DeclStmt *DS, CheckerContext &C) const {}  \
Index: clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
===
--- clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
+++ clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
@@ -26,7 +26,7 @@
 
 class FalsePositiveGenerator : public Checker {
   using Self = FalsePositiveGenerator;
-  const BuiltinBug FalsePositiveGeneratorBug{this, "FalsePositiveGenerator"};
+  const BugType FalsePositiveGeneratorBug{this, "FalsePositiveGenerator"};
   using HandlerFn = bool (Self::*)(const CallEvent &Call,
CheckerContext &) const;
   CallDescriptionMap Callbacks = {
Index: clang/unittests/StaticAnalyzer/CallEventTest.cpp
===
--- clang/unittests/StaticAnalyzer/CallEventTest.cpp
+++ clang/unittests/StaticAnalyzer/CallEventTest.cpp
@@ -36,11 +36,11 @@
 }
 
 class CXXDeallocatorChecker : public Checker {
-  std::unique_ptr BT_uninitField;
+  std::unique_ptr BT_uninitField;
 
 public:
   CXXDeallocatorChecker()
-  : BT_uninitField(new BuiltinBug(this, "CXXDeallocator")) {}
+  : BT_uninitField(new BugType(this, "CXXDeallocator")) {}
 
   void checkPreCall(const CallEvent &Call, CheckerContext &C) const {
 const auto *DC = dyn_cast(&Call);
Index: clang/lib/StaticAnalyzer/Core/BugReporter.cpp
===

[PATCH] D158855: [analyzer][NFC] Remove useless class BuiltinBug

[Balázs Benics via Phabricator via cfe-commits]

steakhal accepted this revision.
steakhal added a comment.
This revision is now accepted and ready to land.

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!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158855

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158855: [analyzer][NFC] Remove useless class BuiltinBug

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy marked an inline comment as done.
donat.nagy added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/BoolAssignmentChecker.cpp:27
   class BoolAssignmentChecker : public Checker< check::Bind > {
-mutable std::unique_ptr BT;
+mutable std::unique_ptr BT;
 void emitReport(ProgramStateRef state, CheckerContext &C,

xazax.hun wrote:
> In case you are down to some additional cleanup efforts, we no longer need 
> lazy initialization for `BugType`s. 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. 
Great news! I'll keep it in mind and I'll try to do this clean-up if I have 
enough free time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158855

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158855: [analyzer][NFC] Remove useless class BuiltinBug

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/BoolAssignmentChecker.cpp:27
   class BoolAssignmentChecker : public Checker< check::Bind > {
-mutable std::unique_ptr BT;
+mutable std::unique_ptr BT;
 void emitReport(ProgramStateRef state, CheckerContext &C,

In case you are down to some additional cleanup efforts, we no longer need lazy 
initialization for `BugType`s. 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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158855

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158855: [analyzer][NFC] Remove useless class BuiltinBug

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added inline comments.
Herald added a subscriber: ormris.



Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:35-36
 public Checker {
-  mutable std::unique_ptr BT;
+  mutable std::unique_ptr BT;
   mutable std::unique_ptr TaintBT;
 

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158855

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158855: [analyzer][NFC] Remove useless class BuiltinBug

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy created this revision.
donat.nagy added reviewers: Szelethus, steakhal, gamesh411, NoQ.
Herald added subscribers: manas, ASDenysPetrov, martong, dkrupp, 
mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun.
Herald added a project: All.
donat.nagy requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D158855

Files:
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugType.h
  clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
  clang/lib/StaticAnalyzer/Checkers/BoolAssignmentChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/CastSizeChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/DebugCheckers.cpp
  clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/FixedAddressChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ObjCAtSyncChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/PointerSubChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ReturnPointerRangeChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ReturnUndefChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/TestAfterDivZeroChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/UndefBranchChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/UndefinedArraySubscriptChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp
  
clang/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/VforkChecker.cpp
  clang/lib/StaticAnalyzer/Core/BugReporter.cpp
  clang/unittests/StaticAnalyzer/CallEventTest.cpp
  clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
  clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp

Index: clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
===
--- clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
+++ clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
@@ -89,8 +89,8 @@
 
 class CheckerRegistrationOrderPrinter
 : public Checker> {
-  std::unique_ptr BT =
-  std::make_unique(this, "Registration order");
+  std::unique_ptr BT =
+  std::make_unique(this, "Registration order");
 
 public:
   void checkPreStmt(const DeclStmt *DS, CheckerContext &C) const {
@@ -125,8 +125,7 @@
 
 #define UNITTEST_CHECKER(CHECKER_NAME, DIAG_MSG)   \
   class CHECKER_NAME : public Checker> {  \
-std::unique_ptr BT =   \
-std::make_unique(this, DIAG_MSG);  \
+std::unique_ptr BT = std::make_unique(this, DIAG_MSG);   \
\
   public:  \
 void checkPreStmt(const DeclStmt *DS, CheckerContext &C) const {}  \
Index: clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
===
--- clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
+++ clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
@@ -26,7 +26,7 @@
 
 class FalsePositiveGenerator