NoQ added a comment. Btw, did you try running this patch against big projects? 'Cause it's a high-impact change and we'd have to be careful with it.
I ran it on LLVM real quick and noticed that 97.3% (!) of reports contained at least one of the new notes, with number of HTML path pieces increased by 23.7% on average (note that the impact on plist reports would be much higher because they don't have any text for gray pieces). I'll try to get back with some actual hand-picked examples to demonstrate my impressions. My overall feel was that it increases quality of life quite a bit by reducing the amount of time spent scrolling and jumping around the report - the necessary information becomes available exactly where you need it and it's wonderful. At the same time, i noticed a few corner-cases where the new pieces were duplicating the existing information or otherwise not helpful: F8520276: operator_question_mark.png <https://reviews.llvm.org/F8520276> F8520282: operator_question_mark.html <https://reviews.llvm.org/F8520282> F8520283: more_repeats.png <https://reviews.llvm.org/F8520283> F8520285: more_repeats.html <https://reviews.llvm.org/F8520285> This doesn't look like a fundamental problem to me; it should be possible to improve upon these corner-cases by varying note messages; probably it also makes sense to skip some of them when they're clearly saying the same info 10 times in a row, but i'm not sure it's easy to decide where to apply that. Because it might be a large amount of work, i'm thinking about committing this new feature off-by-default first (eg., `-analyzer-config knowing-notes=[false|true]`) and then doing follow-up commits (you already have quite a bit!) to make sure it's polished when we turn it on for everyone, WDYT? ================ Comment at: include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h:169-173 + // If the constraint information is changed between the current and the + // previous program state we assuming the newly seen constraint information. + // If we cannot evaluate the condition (and the constraints are the same) + // the analyzer has no information about the value and just assuming it. + bool IsAssuming; ---------------- Does this absolutely need to be a member variable? Maybe pass it down the stack, like `tookTrue`? I understand that there are already a lot of flags passed down, but global mutable state is still more dangerous and can easily get out of hand. ================ Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:187-190 + // The value may hard-coded. + SVal HardCodedSVal = State->getSVal(CondVarExpr, LCtx); + if (auto HardCodedCI = HardCodedSVal.getAs<nonloc::ConcreteInt>()) + return &HardCodedCI->getValue(); ---------------- I don't see this triggered on tests. It looks to me that this function is for now only applied to `DeclRefExpr`s that are always glvalues and therefore should never be evaluated to a `nonloc` value. ================ Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1991-1992 return std::make_shared<PathDiagnosticEventPiece>( - Loc, tookTrue ? GenericTrueMessage : GenericFalseMessage); + Loc, IsAssuming ? tookTrue ? AssumingTrueMessage : AssumingFalseMessage + : tookTrue ? TrueMessage : FalseMessage); } ---------------- I advocate for more parentheses :) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53076/new/ https://reviews.llvm.org/D53076 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
