NoQ added inline comments.
================ Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:2219 + // cannot be fixed... + eraseVarsForUnfixableGroupMates(FixItsForVariable, VarGrpMgr); + // Now `FixItsForVariable` gets further reduced: a variable is in ---------------- NoQ wrote: > Architecturally speaking, I think I just realized something confusing about > our code. > > We already have variable groups well-defined at the Strategy phase, i.e. > before we call `getFixIts()`, but then `getFixIts()` continues to reason > about all variables collectively and indiscriminately. It continues to use > entities such as the `FixItsForVariable` map which contain fixits for > variables from *all* groups, not just the ones that are currently relevant. > Then it re-introduces per-group data structures such as `ParmsNeedFixMask` on > an ad-hoc basis, and it tries to compute them this way using the global, > indiscriminate data structures. > > I'm starting to suspect that the code would start making a lot more sense if > we invoke `getFixIts()` separately for each variable group. So that each such > invocation produced a single collective fixit for the group, or failed doing > so. > > This way we might be able to avoid sending steganographic messages through > `FixItsForVariable`, but instead say directly "these are the variables that > we're currently focusing on". It is the responsibility of the `Strategy` > class to answer "should this variable be fixed?"; we shouldn't direct that > question to any other data structures. > > And if a group fails at any point, just early-return `None` and proceed > directly to the next getFixIts() invocation for the next group. We don't need > to separately record which individual variables have failed. In particular, > `eraseVarsForUnfixableGroupMates()` would become a simple early return. > > It probably also makes sense to store groups themselves inside the `Strategy` > class. After all, fixing variables together is a form of strategy. (I don't think this needs to be addressed in the current patch, but this could help us untangle the code in general.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156762/new/ https://reviews.llvm.org/D156762 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
