[PATCH] D156474: [-Wunsafe-buffer-usage][NFC] Slightly refactor and optimize the code

[Ziqing Luo via Phabricator via cfe-commits]

ziqingluo-90 added inline comments.


================
Comment at: clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h:25
+
+class VariableGroupsManager {
+public:
----------------
Use the `VariableGroupsManager` class as an interface to clients.  It hides the details about how groups are implemented (which may involve several containers). 


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:1143
+           // order is deterministic:
+           CompareNode<VarDecl>>
+      byVar;
----------------
To make the order of variables in every group deterministic.


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:2179
+      if (FixItsForVariable.count(GrpMate))
+        FinalFixItsForVariable[Var].insert(FinalFixItsForVariable[Var].end(),
+                                           FixItsForVariable[GrpMate].begin(),
----------------
Instead of calling `fixVariable` to generate fix-its for group mates, directly copying those fix-its from the `FixItsForVariable` container.

At this point,  `FixItsForVariable` is complete. That is, it maps every variable to its own fix-its (excluding fix-its of group mates).


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:2198
+//  Manages variable groups:
+class VariableGroupsManagerImpl : public VariableGroupsManager {
+  const std::vector<VarGrpTy> Groups;
----------------
An implementation of `VariableGroupsManager`:  keep a unique copy of each group in `Groups` and use their indexes for access.

Note that `getGroupOfVar` assumes that `Var` must in the map.  This is guaranteed by the changed made in [[ https://reviews.llvm.org/D155524  | D155524 ]]---only "Fixables" associated to variables in the final graph(es) will stay and be fixed.    The key set of the `VarGrpMap` is the set of variables in the final graph(es).   Therefore, after the graph(es) are computed and "Fixables" are pruned,  `VarGrpMap` is the variable domain that we will deal with.


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:2309
   // Fixpoint iteration for pointer assignments
-  using DepMapTy = DenseMap<const VarDecl *, std::set<const VarDecl *>>;
+  using DepMapTy = DenseMap<const VarDecl *, llvm::SetVector<const VarDecl *>>;
   DepMapTy DependenciesMap{};
----------------
Use `SetVector` to make order deterministic.


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:2369
-      for (const VarDecl * V : VarGroup) {
-        if (UnsafeOps.byVar.find(V) != UnsafeOps.byVar.end()) {
-          VariableGroupsMap[V] = VarGroup;
----------------
I removed this guard so that `VarGrpMap` is complete---it has an entry for every variable that we need to deal with later.


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:2394
+      for (const VarDecl *V : VarGroup) {
+        VarGrpMap[V] = GrpIdx;
       }
----------------
Mapping variables to group indexes instead of groups themselves to avoid copies.


================
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2168
+  // itself:
+  std::string listVariableGroupAsString(
+      const VarDecl *VD, const ArrayRef<const VarDecl *> &VarGroupForVD) const {
----------------
Factor this procedure out to a method for easy re-use later.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156474/new/

https://reviews.llvm.org/D156474