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 by the target triple.
----------------
Szelethus wrote:
> 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?).
Well, I have removed it instead. Makes no sense, you are right.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:145
+
+  // Get the concrete return value.
+  Optional<const bool *> RawConcreteValue = CDM.lookup(*Call);
----------------
NoQ wrote:
> NoQ wrote:
> > I think you can squeeze all this code into one big `isInvariantBreak(Call, 
> > ReturnV)` and re-use it across both methods.
> I think it would be more precise to talk about "expected"/"actual" return 
> value. The actual return value may be either concrete (i.e., 
> `nonloc::ConcreteInt`) or symbolic (i.e., `nonloc::SymbolVal`).
> 
> Also, there's a relatively famous rule of a thumb for making good comments: 
> "comments shouldn't explain what does the code do, but rather why does it do 
> it". Instead of these comments i'd rather have one large comment at the 
> beginning of `checkEndFunction` explaining what are we trying to do here.
Great advice, thanks you!


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

Reply via email to