================
@@ -112,25 +112,30 @@ class NullabilityChecker
   void printState(raw_ostream &Out, ProgramStateRef State, const char *NL,
                   const char *Sep) const override;
 
-  enum CheckKind {
-    CK_NullPassedToNonnull,
-    CK_NullReturnedFromNonnull,
-    CK_NullableDereferenced,
-    CK_NullablePassedToNonnull,
-    CK_NullableReturnedFromNonnull,
-    CK_NumCheckKinds
+  // FIXME: This enumeration of checker parts is extremely similar to the
+  // ErrorKind enum. It would be nice to unify them to simplify the code.
+  enum : CheckerPartIdx {
+    NullPassedToNonnullChecker,
+    NullReturnedFromNonnullChecker,
+    NullableDereferencedChecker,
+    NullablePassedToNonnullChecker,
+    NullableReturnedFromNonnullChecker,
+    NumCheckerParts
   };
 
-  bool ChecksEnabled[CK_NumCheckKinds] = {false};
-  CheckerNameRef CheckNames[CK_NumCheckKinds];
-  mutable std::unique_ptr<BugType> BTs[CK_NumCheckKinds];
-
-  const std::unique_ptr<BugType> &getBugType(CheckKind Kind) const {
-    if (!BTs[Kind])
-      BTs[Kind].reset(new BugType(CheckNames[Kind], "Nullability",
-                                  categories::MemoryError));
-    return BTs[Kind];
-  }
+  // FIXME: Currently the `Description` fields of these `BugType`s are all
+  // identical ("Nullability") -- they should be more descriptive than this.
+  BugType BugTypes[NumCheckerParts] = {
+      {this, NullPassedToNonnullChecker, "Nullability",
+       categories::MemoryError},
+      {this, NullReturnedFromNonnullChecker, "Nullability",
+       categories::MemoryError},
+      {this, NullableDereferencedChecker, "Nullability",
+       categories::MemoryError},
+      {this, NullablePassedToNonnullChecker, "Nullability",
+       categories::MemoryError},
+      {this, NullableReturnedFromNonnullChecker, "Nullability",
+       categories::MemoryError}};
----------------
NagyDonat wrote:

> Correct me if I'm wrong, but we don't really need a BugType array.

Yep, you're completely right, and I should have noticed this -- e.g. 
`DivZeroChecker` also declares the bug types as individual variables (and not 
arrays).

> We also don't really need a checker part enum either. [...]

When I designed the current framework I implicitly assumed that we will need a 
checker part _identifier_ type which acts as an index/key for a (perhaps 
associative) array containing the names / registration status, because this was 
a common element in all the individual checkers plus the old abandoned 
`SubChecker` draft commit that was written by Kristóf Umann several years ago. 
(I spent lots of time on thinking about details _within this box_, but I didn't 
consider out-of-the box ideas.) Now that I think about your suggestion, I see 
that it could lead to a simpler alternative framework.

The operations that a checker part type needs to support:
- The constructor of `BugType` is called from the constructor of a checker and 
needs to take a checker part argument (so the checker part should be available 
at this point).
- `BugType` stores this checker part as a data member (so all checkers should 
use the same `CheckerPart` type). 
- `CheckerManager::registerChecker` takes a checker part as an argument (in 
fact, currently as a template argument, but this may change) and binds the name 
injected from `Checkers.td` to it. (Binding the name implicitly marks the part 
as enabled, because enabled parts need to have names, while others don't have 
names.)
- The enabled/disabled status of a checker part may be queried within various 
checker callbacks.
- The name of the checker part is queried by the bug types associated with it 
and may be queried elsewhere within the checker callbacks.
- `getTagDescription()` currently iterates over the checker parts to find one 
that has a name. This is the only case where _iteration over checker parts_ 
happen, so perhaps it should be replaced by an alternative implementation.

These could be satisfied by a checker part type that looks like
```c++
class CheckerPart {
  std::optional<CheckerNameRef> Name;
public:
  void enable(CheckerManager &Mgr) {
    assert(!Name && "Checker part registered twice!");
    Name = Mgr.getCurrentCheckerName();
  }    
  bool isEnabled() { return static_cast<bool>(Name); }
  CheckerNameRef getName() { return *Name; }
}

class FooOrBarChecker {
public:
  CheckerPart Foo, Bar;
private:
  BugType FooBug{Foo, "Foo does not work"};
  // ...
}

void registerFooChecker(CheckerManager &Mgr) {
  Mgr.registerChecker<FooOrBarChecker>()->Foo.enable(Mgr);
}
```

https://github.com/llvm/llvm-project/pull/132250
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to