xazax.hun added inline comments.
================
Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:651-652
+ } else if (StoreSite->getLocation().getAs<BlockEntrance>()) {
+ os << "Reach the max loop limit.";
+ os << " Assigning a conjured symbol";
+ if (R->canPrintPretty()) {
----------------
zaks.anna wrote:
> xazax.hun wrote:
> > zaks.anna wrote:
> > > MTC wrote:
> > > > NoQ wrote:
> > > > > This is user-facing text, and users shouldn't know about conjured
> > > > > symbols, and "max" shouldn't be shortened, and i'm not sure what
> > > > > else. I'd probably suggest something along the lines of "Contents of
> > > > > <...> are wiped", but this is still not good enough.
> > > > >
> > > > > Also could you add a test that displays this note? I.e. with
> > > > > `-analyzer-output=text`.
> > > > Thanks for your review.
> > > >
> > > > You are right, whether this information should be displayed to the user
> > > > is a question worth discussing.
> > > I am not convinced that we need to print this information to the user.
> > > The problem here is that it leaks internal implementation details about
> > > the analyzer. The users should not know about "loop limits" and
> > > "invalidation" and most of the users would not even know what this means.
> > > I can see how this is useful to the analyzer developers for debugging the
> > > analyzer, but not to the end user.
> > >
> > While we might not want to expose this to the user this can be really
> > useful to understand what the analyzer is doing when we debugging a false
> > positive finding. Maybe it would be worth to introduce a developer mode or
> > verbose mode for those purposes. What do you think?
> I'd be fine with that in theory, though the downside is that it would pollute
> the code a bit. One trick that's often used to better understand a report
> when debugging is to remove the path note pruning (by passing a flag). I am
> not sure if that approach can be extended for this case. Ex: maybe we could
> have special markers on the notes saying that they are for debug purposes
> only and have the pruning remove them.
>
> By the way, is this change related to the other change from this patch?
I agree that we should just commit this without the message to fix the assert.
The "debug message" could be addressed in a separate patch.
https://reviews.llvm.org/D37187
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
