NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land.
I see you've started to address the big comment in D157441 <https://reviews.llvm.org/D157441>, so, LGTM, thanks a lot for splitting stuff up! I have one tiny stylistic nitpick. ================ Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:2158-2159 + [&FixItsForVariable](const VarDecl *GrpMember) -> bool { + return FixItsForVariable.find(GrpMember) == + FixItsForVariable.end(); + })) { ---------------- (also indentation does a lot of heavy lifting here, maybe let's stash the `any_of` into a variable?) (also we have `using namespace llvm` at the beginning of the file, so `llvm::` might be redundant) ================ Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:2219 + // cannot be fixed... + eraseVarsForUnfixableGroupMates(FixItsForVariable, VarGrpMgr); + // Now `FixItsForVariable` gets further reduced: a variable is in ---------------- ziqingluo-90 wrote: > NoQ wrote: > > 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.) > This makes absolute sense! Each group is independent for fix-it generation. > Moreover, when we support more strategy kinds, the constraint solving for a > proper strategy will also be group-based. > the constraint solving for a proper strategy will also be group-based. Hmm, my head-nomenclature (like head-canon but nomenclature) says that grouping is a sub-task of solving for strategy. I.e., we take "strategy constraints" of the form ``` 'p' is any safe type => 'q' is any safe type ``` and solve these constraints by crawling through the implication graph. But all the solving subtasks below grouping should probably indeed be separate! 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
