NoQ added inline comments.
================ Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:406 +const ExplodedNode *StreamChecker::getAcquisitionSite(const ExplodedNode *N, + SymbolRef StreamSym, ---------------- balazske wrote: > balazske wrote: > > NoQ wrote: > > > NoQ wrote: > > > > Ok, so this is a tiny auxiliary visitor. > > > > > > > > I wish we could set uniqueing location post-factum from within the note > > > > tag. Unfortunately we can't because notes are generated after uniqueing > > > > :( > > > > > > > > I'd like you to experiment with tracking this information as part of > > > > the state instead so that you didn't need to perform an additional > > > > scan. I'd be happy if it helps you avoid performing this additional > > > > pass. > > > > > > > > I.e., when you're opening the file, add all the information you need > > > > for building your uniqueing location into the stream state (not the > > > > node though, just the program point or something like that). Then > > > > retrieve it in O(1) when you're emitting the report. > > > Another thing we could try is to implement such post-processing pass > > > globally for all checkers. I.e., instead of an optional uniqueing > > > location accept an optional lambda that looks at a node and answers > > > whether this node should be used as a uniqueing location. Then before > > > report deduplication scan each report that supplies such lambda and > > > update its uniqueing location. That'll probably require some work on > > > BugReporter but that sounds like a nice way to avoid all the boilerplate. > > Storing the "acquisition site" in the state is the natural way of doing > > this. Probably I should not think that existing checkers do things the best > > way, this function is taken from `FuchsiaHandleChecker` (or here it has a > > specific reason?). And not storing the data in the state is a bit less > > memory consumption if this matters. > We should check how many checkers can benefit from such a "uniqueing location > callback". Normally the checker should know what the uniqueing location is, > at least when the bug report is created. The location is naturally obtained > from the state that the checker maintains, at least if we want to avoid scans > like `getAcquisitionSite`. > Probably I should not think that existing checkers do things the best way Well, if you ever find yourself copy-pasting a large chunk of code from a different checker and using it almost unchanged, it's a good indication that the checker API needs to be improved. You can't do things "the best way" in a single checker in isolation, it's a collective effort. You're a programmer, you can change everything, no need to be confined in your checker. > We should check how many checkers can benefit from such a "uniqueing location > callback". Normally the checker should know what the uniqueing location is, > at least when the bug report is created. No, i don't think it ever happens automagically. It's either tracked in the state specifically for that purpose or scanned backwards. So i'd rather believe that every checker that has non-default uniqueing locations will benefit from such facility. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81407/new/ https://reviews.llvm.org/D81407 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
