[PATCH] D156762: [-Wunsafe-buffer-usage][NFC] Refactor `getFixIts`---where fix-its are generated

[Artem Dergachev via Phabricator via cfe-commits]

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