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

[Kristóf Umann via Phabricator via cfe-commits]

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 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 target triple.
----------------
This isn't true: the user may decide to only enable non-pathsensitive checkers.

I think the comment should rather state that these whether these checkers are enabled shouldn't be explicitly specified, unless in **extreme** circumstances (causes a crash in a release build?).


================
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 &&Cb) {
-    return Eng.getNoteTags().makeNoteTag(std::move(Cb));
+  const NoteTag *getNoteTag(NoteTag::Callback &&Cb, bool IsPrunable = false) {
+    return Eng.getNoteTags().makeNoteTag(std::move(Cb), IsPrunable);
   }
----------------
Hmm, we use interestingness (`markInteresting()`) already to determine whether we should prune events or not, maybe we could (in the long term) try to make these mechanisms work in harmony.

In any case, could you please add comments about the new parameter to the class doc? :)


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

https://reviews.llvm.org/D63915