[PATCH] D63915: [analyzer] ReturnValueChecker: Model the guaranteed boolean return value of function calls

This revision was automatically updated to reflect the committed changes. Closed by commit rL365103: [analyzer] ReturnValueChecker: Model the guaranteed boolean return value of… (authored by Charusso, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed

[PATCH] D63915: [analyzer] ReturnValueChecker: Model the guaranteed boolean return value of function calls

Charusso updated this revision to Diff 207939. Charusso marked 9 inline comments as done. Charusso added a comment. - Done. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63915/new/ https://reviews.llvm.org/D63915 Files: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

[PATCH] D63915: [analyzer] ReturnValueChecker: Model the guaranteed boolean return value of function calls

Charusso added a comment. Thanks for the reviews! In D63915#1569410 , @Szelethus wrote: > This checker isn't in alpha -- did you evaluate it on LLVM? Other than that, > looks great! Yes, it is made for LLVM and tested out 4 times. Thanks! CHANGES

[PATCH] D63915: [analyzer] ReturnValueChecker: Model the guaranteed boolean return value of function calls

Szelethus added a comment. This checker isn't in alpha -- did you evaluate it on LLVM? Other than that, looks great! Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:1119 - const T *lookup(const CallEvent ) const { + Optional lookup(const

[PATCH] D63915: [analyzer] ReturnValueChecker: Model the guaranteed boolean return value of function calls

NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. I think i like it now! Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:1119 - const T *lookup(const CallEvent ) const { + Optional lookup(const

[PATCH] D63915: [analyzer] ReturnValueChecker: Model the guaranteed boolean return value of function calls

Charusso added a comment. Thanks for the reviews! The remaining question is: do we want to use `Optional<>` in the `CallDescriptionMap::lookup()`? Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:98-100 +// The APIModeling package is for checkers that model

[PATCH] D63915: [analyzer] ReturnValueChecker: Model the guaranteed boolean return value of function calls

Charusso updated this revision to Diff 207862. Charusso marked 12 inline comments as done. Charusso added a comment. - More fix. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63915/new/ https://reviews.llvm.org/D63915 Files: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

[PATCH] D63915: [analyzer] ReturnValueChecker: Model the guaranteed boolean return value of function calls

Szelethus added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:98-100 +// The APIModeling package is for checkers that model APIs. These checkers are +// always turned on; this package is intended for API modeling that is not +// controlled

[PATCH] D63915: [analyzer] ReturnValueChecker: Model the guaranteed boolean return value of function calls

xazax.hun added a comment. Some nits inline, note that this was just a partial review. Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:14 + +#include "clang/Driver/DriverDiagnostic.h" +#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"

[PATCH] D63915: [analyzer] ReturnValueChecker: Model the guaranteed boolean return value of function calls

Charusso marked 5 inline comments as done. Charusso added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:98-100 +// The APIModeling package is for checkers that model APIs. These checkers are +// always turned on; this package is intended

[PATCH] D63915: [analyzer] ReturnValueChecker: Model the guaranteed boolean return value of function calls

NoQ added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:98-100 +// The APIModeling package is for checkers that model APIs. These checkers are +// always turned on; this package is intended for API modeling that is not +// controlled by the

[PATCH] D63915: [analyzer] ReturnValueChecker: Model the guaranteed boolean return value of function calls

Charusso added a comment. In D63915#1568166 , @Szelethus wrote: > This checker seems to only check LLVM functions, but doesn't check whether > these methods lie in the LLVM namespace. Is this intended? Thanks for the reviews! They are not in the `llvm`

[PATCH] D63915: [analyzer] ReturnValueChecker: Model the guaranteed boolean return value of function calls

Charusso updated this revision to Diff 207851. Charusso marked 8 inline comments as done. Charusso added a comment. - Fix. - Document `NoteTag`. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63915/new/ https://reviews.llvm.org/D63915 Files:

[PATCH] D63915: [analyzer] ReturnValueChecker: Model the guaranteed boolean return value of function calls

NoQ added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:98-100 +// The APIModeling package is for checkers that model APIs. These checkers are +// always turned on; this package is intended for API modeling that is not +// controlled by the

[PATCH] D63915: [analyzer] ReturnValueChecker: Model the guaranteed boolean return value of function calls

Szelethus added a comment. This checker seems to only check LLVM functions, but doesn't check whether these methods lie in the LLVM namespace. Is this intended? Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:98-100 +// The APIModeling package is for

[PATCH] D63915: [analyzer] ReturnValueChecker: Model the guaranteed boolean return value of function calls

Charusso added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h:210 ///the default tag for the checker will be used. + /// @param IsPrunable Whether the note is prunable. ExplodedNode * It makes

[PATCH] D63915: [analyzer] ReturnValueChecker: Model the guaranteed boolean return value of function calls

Charusso added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:98-100 +// The APIModeling package is for checkers that model APIs. These checkers are +// always turned on; this package is intended for API modeling that is not +// controlled

[PATCH] D63915: [analyzer] ReturnValueChecker: Model the guaranteed boolean return value of function calls

Charusso updated this revision to Diff 207703. Charusso marked 10 inline comments as done. Charusso added a comment. - Fix. - Refactor. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63915/new/ https://reviews.llvm.org/D63915 Files:

[PATCH] D63915: [analyzer] ReturnValueChecker: Model the guaranteed boolean return value of function calls

NoQ added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:107 + + // If the invariant is broken we do not return the concrete value. +if (IsInvariantBreak) Something's wrong with formatting.

[PATCH] D63915: [analyzer] ReturnValueChecker: Model the guaranteed boolean return value of function calls

NoQ added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h:228-229 /// a bug, you can add notes as you add your transitions. - const NoteTag *getNoteTag(NoteTag::Callback &) { -return

[PATCH] D63915: [analyzer] ReturnValueChecker: Model the guaranteed boolean return value of function calls

Szelethus added a comment. I'd love to chip in later, if you don't mind, but here is just a couple things that caught my mind that I'd like to share before falling asleep ;) Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:98-100 +// The APIModeling package

[PATCH] D63915: [analyzer] ReturnValueChecker: Model the guaranteed boolean return value of function calls

Charusso added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:120 +<< (Value ? "true" : "false") +<< " according to the LLVM coding standard, but it returns " +<< (Value ? "false" : "true");

[PATCH] D63915: [analyzer] ReturnValueChecker: Model the guaranteed boolean return value of function calls

Charusso updated this revision to Diff 207668. Charusso marked 7 inline comments as done. Charusso added a comment. - Create `FunctionExitPoint` diagnostics. - Fix. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63915/new/ https://reviews.llvm.org/D63915 Files:

[PATCH] D63915: [analyzer] ReturnValueChecker: Model the guaranteed boolean return value of function calls

NoQ added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:71 + SVal RetV = Call.getReturnValue(); + Optional RetDV = RetV.getAs(); + if (!RetDV.hasValue()) `auto` is encouraged here. CHANGES SINCE LAST ACTION

[PATCH] D63915: [analyzer] ReturnValueChecker: Model the guaranteed boolean return value of function calls

NoQ added a comment. This is starting to look great, thanks! Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:96 + + Out << '\'' << Name << "' always returns " + << (Value ? "true" : "false"); Let's omit the word

[PATCH] D63915: [analyzer] ReturnValueChecker: Model the guaranteed boolean return value of function calls

NoQ added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:86 + std::string Name = ""; + Name += dyn_cast(Call.getDecl())->getParent()->getName(); + Name += "::"; Either `cast<>` or check for null.

[PATCH] D63915: [analyzer] ReturnValueChecker: Model the guaranteed boolean return value of function calls

Charusso updated this revision to Diff 207458. Charusso added a comment. - I do not like `Optional` anymore. - More simple notes. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63915/new/ https://reviews.llvm.org/D63915 Files: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

[PATCH] D63915: [analyzer] ReturnValueChecker: Model the guaranteed boolean return value of function calls

Charusso marked 2 inline comments as done. Charusso added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:82 + +Out << '\'' << Name << "' always returns " +<< (*Value ? "true" : "false"); NoQ wrote: >

[PATCH] D63915: [analyzer] ReturnValueChecker: Model the guaranteed boolean return value of function calls

NoQ added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:82 + +Out << '\'' << Name << "' always returns " +<< (*Value ? "true" : "false"); Charusso wrote: > NoQ wrote: > > Let's mention the class name as

[PATCH] D63915: [analyzer] ReturnValueChecker: Model the guaranteed boolean return value of function calls

Charusso updated this revision to Diff 207456. Charusso marked 11 inline comments as done. Charusso added a comment. - Fix. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63915/new/ https://reviews.llvm.org/D63915 Files: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

[PATCH] D63915: [analyzer] ReturnValueChecker: Model the guaranteed boolean return value of function calls

Charusso added a comment. In D63915#1563049 , @NoQ wrote: > Aha, nice, thanks for adding a description, it is a very good thing to do. > Like, every commit should be obvious. In some of my patches I have not added a description because they are so tiny

[PATCH] D63915: [analyzer] ReturnValueChecker: Model the guaranteed boolean return value of function calls

NoQ added inline comments. Comment at: clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp:36 -const bool *LookupResult = CDM.lookup(*Call); +Optional LookupResult = CDM.lookup(*Call); // Check that we've found the function in the map I updated

[PATCH] D63915: [analyzer] ReturnValueChecker: Model the guaranteed boolean return value of function calls

NoQ added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:1119 - const T *lookup(const CallEvent ) const { + Optional lookup(const CallEvent ) const { // Slow path: linear lookup. I hope `T` never gets too