NoQ added inline comments.
================ Comment at: clang/lib/StaticAnalyzer/Core/BugReporter.cpp:2260-2261 + InterestingSymbols.erase(sym); + if (const auto *meta = dyn_cast<SymbolMetadata>(sym)) + markNotInteresting(meta->getRegion()); +} ---------------- balazske wrote: > balazske wrote: > > NoQ wrote: > > > You're saying, e.g., "If string length is not interesting then neither is > > > the string itself". Or, dunno, "If the raw pointer value is not > > > interesting then neither is a smart pointer that was holding it". > > > > > > I'm not sure about that. I'm pretty sure that no checkers are currently > > > affected by this code but I still don't understand your point. > > > > > > I don't understand the original code in `markInteresting()` either; do we > > > have even one test to cover it? > > > > > > Also note that what you've written is not a correct negation of the > > > original code. The correct negation (that would keep the region-metadata > > > relationship in sync as originally intended) would be "if the region > > > loses interestingness, so does the metadata". Or it has to go both ways. > > > I'm still not sure if any of this matters though. > > > > > > Maybe we should eliminate these extra clauses entirely. If you're not > > > willing to investigate whether this is all dead code or it actually does > > > something, I'd like to suggest a "FIXME: Is this necessary?" (or > > > something like that) both here and in the original code. > > The metadata interestingness was added in rG735724fb1e78 long ago, there is > > not more information. All test pass if the code is removed (but it may have > > impacts on bug paths). Is it not the job of the checker to mark the > > interestingness of objects? I assume that the meaning of > > `meta->getRegion()` is checker dependent (the string or smart pointer or > > something totally different?) and the checker should know what to make > > interesting (or not). The `markInteresting()` has no test (what I could > > extend here), it may be implicitly tested by the other checker tests. > The mark of associated region as interesting may not work correct if there > are multiple metadata symbols for the same region, which should be a possible > case. Specially the remove of interestingness is not correct in this way. So > I would remove the `if (const auto *meta = dyn_cast<SymbolMetadata>(sym))` > part. > A better approach can be to make the interestingness depend totally on the > associated region for metadata symbol. If a symbol is metadata, its > interestingness is that of the region. And only the region is inserted into > the map. `isInteresting` can be updated for this purpose. > I assume that the meaning of `meta->getRegion()` is checker dependent I completely agree with your reasoning. One way or another, that old commit didn't come with a test, or any explanation whatsoever. I think we have all the rights to question it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105637/new/ https://reviews.llvm.org/D105637 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
