NoQ added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1052
+ // to return that every time.
+ if (N->getCFG().isLinear())
+ WouldEventBeMeaningless = true;
----------------
This time i'd rather go for "has exactly 3 blocks", because we're mostly
interested in the CFG being "syntactically linear" rather than "semantically
linear". I.e., if the CFG has an if-statement with a compile-time-constant
condition, i'd rather show the path, because who knows what does the programmer
think about this condition.
================
Comment at: clang/test/Analysis/track-control-dependency-conditions.cpp:185
return true;
return coin(); // tracking-note{{Returning value}}
}
----------------
Szelethus wrote:
> This note is meaningful, the bug would not have occurred if `coin()` wasn't
> assumed to be false.
Mmmmm. Mmmmm. Dunno, this is getting complicated very quickly. Say, if you
replace `return true` with `return false` on the previous line, the note
becomes meaningless again. I don't see a direct connection between
meaningfulness and linearness.
The note in this example is relatively meaningful indeed, but i'm not sure it's
so much meaningful that it's worth the noise. It's still not surprising for me
that `flipCoin()` occasionally returns false. I do believe that it may be
sometimes surprising that `flipCoin()` may return false on the *current path*,
which would make the note meaningful. However, note that we don't prove that it
may return false, we only push the assumption one step deeper, i.e. put the
blame on `coin()` instead of `flipCoin()`, but we still fully trust our
assumption about `coin()` which may have the same problem. And if we had an
actual proof that it may return false, we would have had a concrete-false
return value, so the patch wouldn't apply.
I guess some experimental data would be great to have.
================
Comment at: clang/test/Analysis/uninit-vals.c:181
void testUseHalfPoint() {
- struct Point p = getHalfPoint(); // expected-note{{Calling 'getHalfPoint'}}
- // expected-note@-1{{Returning from
'getHalfPoint'}}
- // expected-note@-2{{'p' initialized here}}
+ struct Point p = getHalfPoint(); // expected-note{{'p' initialized here}}
use(p); // expected-warning{{uninitialized}}
----------------
Szelethus wrote:
> Szelethus wrote:
> > NoQ wrote:
> > > Huh, so there's not even a note in `getHalfPoint()`, just
> > > calling..returning? This definitely needs some attention from
> > > `NoStoreFuncVisitor`.
> > >
> > > Generally, i think this is probably the single place where we do really
> > > want some info about what happens in `getHalfPoint()`. The report that
> > > consists only of "p is initialized..." and "...p is uninitialized" is
> > > pretty weird. Btw, could you write down the full warning text in this
> > > test? How bad this actually is?
> > Wild idea: `UninitializedObjectChecker` exposes it's main logic through a
> > header file, how about we use it when we find a read of an uninitialized
> > region?
> Passed-by-value struct argument contains uninitialized data (e.g., field: 'y')
>
> Quite good imo.
What specific logic are you talking about? You mean it'd scan the structure for
uninitialized fields and present the results of the scan to the user in a note?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64232/new/
https://reviews.llvm.org/D64232
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
