NoQ added a comment. Aha, nice, thanks for adding a description, it is a very good thing to do. Like, every commit should be obvious. Even though i know what's going on, nobody else does, so it's good to explain that this is for modeling certain weird LLVM-specific APIs that intentionally always return true.
I guess this checker should eventually be merged with `StdCLibraryFunctionsChecker` and such. I wonder if it's trivial to convert it to use `CallDescription`s and then extend it trivially to C++ functions. See also D54149 <https://reviews.llvm.org/D54149>. This is one more reason why i don't want to reinvent APINotes: i'm responsible for one more re-inventing. I'll see if i can generalize upon all of these. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:31-37 +namespace { +struct CallTy { + bool TruthValue; + StringRef Name; + Optional<StringRef> Class; +}; +} ---------------- Let's use `CallDescriptionMap<bool>` (it hasn't landed yet but it's almost there, D62441). It should save quite some code; feel free to introduce a convenient constructor if it turns out to be hard to produce an initializer list. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:81 + // Create a note. + const NoteTag *CallTag = C.getNoteTag([Call](BugReport &BR) -> std::string { + SmallString<128> Msg; ---------------- Please make the note prunable, so that it didn't bring in the nested stack frame if it appears in a nested stack frame (cf. D61817) - you might need to add a new flag to `CheckerContext::getNoteTag()` to make this happen. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:89 + + Out << Call->Name << "' always return " + << (Call->TruthValue ? "true" : "false"); ---------------- "returns" ================ Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:98 + SVal RetV = CE.getReturnValue(); + Optional<DefinedOrUnknownSVal> RetDV = RetV.getAs<DefinedOrUnknownSVal>(); + ---------------- `castAs` if you're sure. Generally it's either evaluated conservatively (so the return value is a conjured symbol) or inlined (and in this case returning an undefined value would result in a fatal warning), so return values shouldn't ever be undefined. The only way to obtain an undefined return value is by binding it in `evalCall()`, but even then, i'd rather issue a warning immediately. ================ Comment at: clang/test/Analysis/return-value-guaranteed.cpp:20 +struct Foo { int Field; }; +bool error(); +bool problem(); ---------------- `core.builtin.NoReturnFunctions` reacts on this function. You can easily see it in the explodedgraph dump, as the last sink node in `test_calls::parseFoo` is tagged with that checker. You'll have to pick a different name for testing purposes. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63915/new/ https://reviews.llvm.org/D63915 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
