NoQ added inline comments.
================
Comment at:
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:872-883
+ if (NewState && NewState != State) {
+ StringRef Note = Case.getNote();
+ const NoteTag *Tag = C.getNoteTag(
+ // Sorry couldn't help myself.
+ [Node, Note]() {
+ // Don't emit "Assuming..." note when we ended up
+ // knowing in advance which branch is taken.
----------------
steakhal wrote:
> I thought we mostly have one or two cases. If we have two cases, then the
> constraint should be the opposite of the other one.
>
> If this is the case, when does `NewState && NewState != State` not imply
> `Node->succ_size() > 1` given that `Summary.getCases().size() >= 2`?
Typically this is going to be the case but I wouldn't rely on that. The
constraint solver may get confused (inb4 "system is over constrained"), there
may be other updates to the state in the branch definition which would cause
the state to change.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:879
+ // knowing in advance which branch is taken.
+ return (Node->succ_size() > 1) ? Note.str() : "";
+ },
----------------
balazske wrote:
> balazske wrote:
> > Can other checkers not add successor nodes (that make this check not always
> > correct)?
> Is there a reason against add the note without the word "Assuming" instead of
> no note?
> Can other checkers not add successor nodes (that make this check not always
> correct)?
They'll be chained to the newly created node, not to the same predecessor. Two
checkers adding transitions on the same post-call won't (and shouldn't)
suddenly cause a state split.
> Is there a reason against add the note without the word "Assuming" instead of
> no note?
We generally never have event notes in such cases.
We sometimes have control flow notes such as "`aking true branch`" (arrow
pointing to the branch in plist).
Then, separately from that, we have tracking notes such as "an interesting
value appeared in this variable from this assignment, which also makes the
assigned value interesting" (eg. "`null pointer value assigned to 'p'`") - we
could have a similar note "an interesting value was returned from that function
because that other value had a certain range, which also makes that other value
interesting" - which we should definitely consider.
But simply saying "this function call was completely predictable and we don't
even care what the value is in the first place" doesn't sound useful.
================
Comment at:
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1326-1330
// The locale-specific range.
.Case({ArgumentCondition(0U, WithinRange, {{128, UCharRangeMax}})})
// Is not an unsigned char.
.Case({ArgumentCondition(0U, OutOfRange, Range(0, UCharRangeMax)),
ReturnValueCondition(WithinRange, SingleValue(0))}));
----------------
steakhal wrote:
> Why don't we have notes for these cases?
Like I mentioned in D122285#inline-1169194, I'm not sure these cases should
exist at all. In particular, explaining them to the user is fairly problematic
because justifying them in the first place is problematic.
================
Comment at: clang/test/Analysis/std-c-library-functions-path-notes.c:3
+// RUN: -analyzer-checker=core,apiModeling \
+// RUN: -analyzer-config assume-controlled-environment=false \
+// RUN: -analyzer-output=text
----------------
steakhal wrote:
> This is the default value. What's the benefit of passing this?
I'm not sure what the default value //should// be. I guess I can remove this
and/or move non-`getenv` tests into another file.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D122285/new/
https://reviews.llvm.org/D122285
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
