Author: Donát Nagy Date: 2025-03-18T16:11:43+01:00 New Revision: ea107d5c63e09347b01b250a6211974d7ed57efe
URL: https://github.com/llvm/llvm-project/commit/ea107d5c63e09347b01b250a6211974d7ed57efe DIFF: https://github.com/llvm/llvm-project/commit/ea107d5c63e09347b01b250a6211974d7ed57efe.diff LOG: [NFC][analyzer] Use `CheckerBase::getName` in checker option handling (#131612) The virtual method `ProgramPointTag::getTagDescription` had two very distinct use cases: - It is printed in the DOT graph visualization of the exploded graph (that is, a debug printout). - The checker option handling code used it to query the name of a checker, which relied on the coincidence that in `CheckerBase` this method is defined to be equivalent with `getName()`. This commit switches to using `getName` in the second use case, because this way we will be able to properly support checkers that have multiple (separately named) parts. The method `reportInvalidCheckerOptionName` is extended with an additional overload that allows specifying the `CheckerPartIdx`. The methods `getChecker*Option` could be extended analogously in the future, but they are just convenience wrappers around the variants that directly take `StringRef CheckerName`, so I'll only do this extension if it's needed. Added: Modified: clang/include/clang/Analysis/ProgramPoint.h clang/include/clang/StaticAnalyzer/Core/Checker.h clang/include/clang/StaticAnalyzer/Core/CheckerManager.h clang/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp clang/lib/StaticAnalyzer/Core/CheckerManager.cpp clang/unittests/StaticAnalyzer/AnalyzerOptionsTest.cpp Removed: ################################################################################ diff --git a/clang/include/clang/Analysis/ProgramPoint.h b/clang/include/clang/Analysis/ProgramPoint.h index 1df1f1cb892e4..c40aa3d8ffb72 100644 --- a/clang/include/clang/Analysis/ProgramPoint.h +++ b/clang/include/clang/Analysis/ProgramPoint.h @@ -39,6 +39,9 @@ class ProgramPointTag { public: ProgramPointTag(void *tagKind = nullptr) : TagKind(tagKind) {} virtual ~ProgramPointTag(); + + /// The description of this program point which will be displayed when the + /// ExplodedGraph is dumped in DOT format for debugging. virtual StringRef getTagDescription() const = 0; /// Used to implement 'isKind' in subclasses. diff --git a/clang/include/clang/StaticAnalyzer/Core/Checker.h b/clang/include/clang/StaticAnalyzer/Core/Checker.h index df29be8ba3f62..a54c5bee612f6 100644 --- a/clang/include/clang/StaticAnalyzer/Core/Checker.h +++ b/clang/include/clang/StaticAnalyzer/Core/Checker.h @@ -516,18 +516,11 @@ class CheckerBase : public ProgramPointTag { } StringRef getTagDescription() const override { - // This method inherited from `ProgramPointTag` has two unrelated roles: - // (1) The analyzer option handling logic uses this method to query the - // name of a checker. - // (2) When the `ExplodedGraph` is dumped in DOT format for debugging, - // this is called to attach a description to the nodes. (This happens - // for all subclasses of `ProgramPointTag`, not just checkers.) - // FIXME: Application (1) should be aware of multiple parts within the same - // checker class instance, so it should directly use `getName` instead of - // this inherited interface which cannot support a `CheckerPartIdx`. - // FIXME: Ideally application (2) should return a string that describes the - // whole checker class, not just one of it parts. However, this is only for - // debugging, so returning the name of one part is probably good enough. + // When the ExplodedGraph is dumped for debugging (in DOT format), this + // method is called to attach a description to nodes created by this + // checker _class_. Ideally this should be recognizable identifier of the + // whole class, but for this debugging purpose it's sufficient to use the + // name of the first registered checker part. for (const auto &OptName : RegisteredNames) if (OptName) return *OptName; diff --git a/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h b/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h index 44ae3ebe3d09d..03ffadd346d0b 100644 --- a/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h +++ b/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h @@ -194,6 +194,13 @@ class CheckerManager { /// Emits an error through a DiagnosticsEngine about an invalid user supplied /// checker option value. void reportInvalidCheckerOptionValue(const CheckerBase *C, + StringRef OptionName, + StringRef ExpectedValueDesc) const { + reportInvalidCheckerOptionValue(C, DefaultPart, OptionName, + ExpectedValueDesc); + } + + void reportInvalidCheckerOptionValue(const CheckerBase *C, CheckerPartIdx Idx, StringRef OptionName, StringRef ExpectedValueDesc) const; diff --git a/clang/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp b/clang/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp index 86ef4a5686650..3e0414d00f551 100644 --- a/clang/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp +++ b/clang/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp @@ -154,8 +154,7 @@ StringRef AnalyzerOptions::getCheckerStringOption(StringRef CheckerName, StringRef AnalyzerOptions::getCheckerStringOption(const ento::CheckerBase *C, StringRef OptionName, bool SearchInParents) const { - return getCheckerStringOption( - C->getTagDescription(), OptionName, SearchInParents); + return getCheckerStringOption(C->getName(), OptionName, SearchInParents); } bool AnalyzerOptions::getCheckerBooleanOption(StringRef CheckerName, @@ -178,8 +177,7 @@ bool AnalyzerOptions::getCheckerBooleanOption(StringRef CheckerName, bool AnalyzerOptions::getCheckerBooleanOption(const ento::CheckerBase *C, StringRef OptionName, bool SearchInParents) const { - return getCheckerBooleanOption( - C->getTagDescription(), OptionName, SearchInParents); + return getCheckerBooleanOption(C->getName(), OptionName, SearchInParents); } int AnalyzerOptions::getCheckerIntegerOption(StringRef CheckerName, @@ -199,6 +197,5 @@ int AnalyzerOptions::getCheckerIntegerOption(StringRef CheckerName, int AnalyzerOptions::getCheckerIntegerOption(const ento::CheckerBase *C, StringRef OptionName, bool SearchInParents) const { - return getCheckerIntegerOption( - C->getTagDescription(), OptionName, SearchInParents); + return getCheckerIntegerOption(C->getName(), OptionName, SearchInParents); } diff --git a/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp b/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp index 56b1e44acc1fd..7ae86f133904b 100644 --- a/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp +++ b/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp @@ -50,11 +50,11 @@ bool CheckerManager::hasPathSensitiveCheckers() const { } void CheckerManager::reportInvalidCheckerOptionValue( - const CheckerBase *C, StringRef OptionName, + const CheckerBase *C, CheckerPartIdx Idx, StringRef OptionName, StringRef ExpectedValueDesc) const { getDiagnostics().Report(diag::err_analyzer_checker_option_invalid_input) - << (llvm::Twine() + C->getTagDescription() + ":" + OptionName).str() + << (llvm::Twine(C->getName(Idx)) + ":" + OptionName).str() << ExpectedValueDesc; } diff --git a/clang/unittests/StaticAnalyzer/AnalyzerOptionsTest.cpp b/clang/unittests/StaticAnalyzer/AnalyzerOptionsTest.cpp index aace4b991b5ec..f4871b8560522 100644 --- a/clang/unittests/StaticAnalyzer/AnalyzerOptionsTest.cpp +++ b/clang/unittests/StaticAnalyzer/AnalyzerOptionsTest.cpp @@ -37,26 +37,17 @@ TEST(StaticAnalyzerOptions, SearchInParentPackageTests) { Opts.Config["Outer.Inner:Option2"] = "true"; Opts.Config["Outer:Option2"] = "false"; - struct CheckerOneMock : CheckerBase { - StringRef getTagDescription() const override { - return "Outer.Inner.CheckerOne"; - } - }; - struct CheckerTwoMock : CheckerBase { - StringRef getTagDescription() const override { - return "Outer.Inner.CheckerTwo"; - } - }; + StringRef CheckerOneName = "Outer.Inner.CheckerOne"; + StringRef CheckerTwoName = "Outer.Inner.CheckerTwo"; // CheckerTwo one has Option specified as true. It should read true regardless // of search mode. - CheckerOneMock CheckerOne; - EXPECT_TRUE(Opts.getCheckerBooleanOption(&CheckerOne, "Option")); + EXPECT_TRUE(Opts.getCheckerBooleanOption(CheckerOneName, "Option")); // The package option is overridden with a checker option. - EXPECT_TRUE(Opts.getCheckerBooleanOption(&CheckerOne, "Option", true)); + EXPECT_TRUE(Opts.getCheckerBooleanOption(CheckerOneName, "Option", true)); // The Outer package option is overridden by the Inner package option. No // package option is specified. - EXPECT_TRUE(Opts.getCheckerBooleanOption(&CheckerOne, "Option2", true)); + EXPECT_TRUE(Opts.getCheckerBooleanOption(CheckerOneName, "Option2", true)); // No package option is specified and search in packages is turned off. We // should assert here, but we can't test that. //Opts.getCheckerBooleanOption(&CheckerOne, "Option2"); @@ -64,8 +55,7 @@ TEST(StaticAnalyzerOptions, SearchInParentPackageTests) { // Checker true has no option specified. It should get false when search in // parents turned on. - CheckerTwoMock CheckerTwo; - EXPECT_FALSE(Opts.getCheckerBooleanOption(&CheckerTwo, "Option", true)); + EXPECT_FALSE(Opts.getCheckerBooleanOption(CheckerTwoName, "Option", true)); // In any other case, we should assert, that we cannot test unfortunately. //Opts.getCheckerBooleanOption(&CheckerTwo, "Option"); //Opts.getCheckerBooleanOption(&CheckerTwo, "Option"); @@ -74,21 +64,6 @@ TEST(StaticAnalyzerOptions, SearchInParentPackageTests) { TEST(StaticAnalyzerOptions, StringOptions) { AnalyzerOptions Opts; Opts.Config["Outer.Inner.CheckerOne:Option"] = "StringValue"; - - struct CheckerOneMock : CheckerBase { - StringRef getTagDescription() const override { - return "Outer.Inner.CheckerOne"; - } - }; - - CheckerOneMock CheckerOne; - EXPECT_TRUE("StringValue" == - Opts.getCheckerStringOption(&CheckerOne, "Option")); -} - -TEST(StaticAnalyzerOptions, SubCheckerOptions) { - AnalyzerOptions Opts; - Opts.Config["Outer.Inner.CheckerOne:Option"] = "StringValue"; EXPECT_TRUE("StringValue" == Opts.getCheckerStringOption( "Outer.Inner.CheckerOne", "Option")); } _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
