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

Slightly refactor and optimize the code in preparation for
implementing grouping parameters for a single fix-it.

Reviewed by: NoQ (Artem Dergachev), t-rasmud (Rashmi Mudduluru)

Differential revision: https://reviews.llvm.org/D156474

Author: ziqingluo-90

clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h

@@ -20,7 +20,19 @@
 
 namespace clang {
 
-using DefMapTy = llvm::DenseMap<const VarDecl *, std::vector<const VarDecl *>>;
+using VarGrpTy = std::vector<const VarDecl *>;
+using VarGrpRef = ArrayRef<const VarDecl *>;
+
+class VariableGroupsManager {
+public:
+  VariableGroupsManager() = default;
+  virtual ~VariableGroupsManager() = default;
+  /// Returns the set of variables (including `Var`) that need to be fixed
+  /// together in one step.
+  ///
+  /// `Var` must be a variable that needs fix (so it must be in a group).
+  virtual VarGrpRef getGroupOfVar(const VarDecl *Var) const;
+};
 
 /// The interface that lets the caller handle unsafe buffer usage analysis
 /// results by overriding this class's handle... methods.
@@ -53,7 +65,7 @@ class UnsafeBufferUsageHandler {
   /// all variables that must be fixed together (i.e their types must be changed to the
   /// same target type to prevent type mismatches) into a single fixit.
   virtual void handleUnsafeVariableGroup(const VarDecl *Variable,
-                                         const DefMapTy &VarGrpMap,
+                                         const VariableGroupsManager &VarGrpMgr,
                                          FixItList &&Fixes) = 0;
 
 #ifndef NDEBUG

clang/lib/Analysis/UnsafeBufferUsage.cpp

@@ -1176,7 +1176,11 @@ groupWarningGadgetsByVar(const WarningGadgetList &AllUnsafeOperations) {
 }
 
 struct FixableGadgetSets {
-  std::map<const VarDecl *, std::set<const FixableGadget *>> byVar;
+  std::map<const VarDecl *, std::set<const FixableGadget *>,
+           // To keep keys sorted by their locations in the map so that the
+           // order is deterministic:
+           CompareNode<VarDecl>>
+      byVar;
 };
 
 static FixableGadgetSets
@@ -1382,7 +1386,7 @@ static std::optional<StringRef> getRangeText(SourceRange SR,
                                              const SourceManager &SM,
                                              const LangOptions &LangOpts) {
   bool Invalid = false;
-  CharSourceRange CSR = CharSourceRange::getCharRange(SR.getBegin(), SR.getEnd());
+  CharSourceRange CSR = CharSourceRange::getCharRange(SR);
   StringRef Text = Lexer::getSourceText(CSR, SM, LangOpts, &Invalid);
 
   if (!Invalid)
@@ -2225,7 +2229,7 @@ getFixIts(FixableGadgetSets &FixablesForAllVars, const Strategy &S,
 	  ASTContext &Ctx,
           /* The function decl under analysis */ const Decl *D,
           const DeclUseTracker &Tracker, UnsafeBufferUsageHandler &Handler,
-          const DefMapTy &VarGrpMap) {
+          const VariableGroupsManager &VarGrpMgr) {
   std::map<const VarDecl *, FixItList> FixItsForVariable;
   for (const auto &[VD, Fixables] : FixablesForAllVars.byVar) {
     FixItsForVariable[VD] =
@@ -2261,9 +2265,10 @@ getFixIts(FixableGadgetSets &FixablesForAllVars, const Strategy &S,
       continue;
     }
 
-    const auto VarGroupForVD = VarGrpMap.find(VD);
-    if (VarGroupForVD != VarGrpMap.end()) {
-      for (const VarDecl * V : VarGroupForVD->second) {
+    
+   {
+      const auto VarGroupForVD = VarGrpMgr.getGroupOfVar(VD);
+      for (const VarDecl * V : VarGroupForVD) {
         if (V == VD) {
           continue;
         }
@@ -2275,7 +2280,7 @@ getFixIts(FixableGadgetSets &FixablesForAllVars, const Strategy &S,
 
       if (ImpossibleToFix) {
         FixItsForVariable.erase(VD);
-        for (const VarDecl * V : VarGroupForVD->second) {
+        for (const VarDecl * V : VarGroupForVD) {
           FixItsForVariable.erase(V);
         }
         continue;
@@ -2293,30 +2298,24 @@ getFixIts(FixableGadgetSets &FixablesForAllVars, const Strategy &S,
     }
   }
 
-  for (auto VD : FixItsForVariable) {
-    const auto VarGroupForVD = VarGrpMap.find(VD.first);
-    const Strategy::Kind ReplacementTypeForVD = S.lookup(VD.first);
-    if (VarGroupForVD != VarGrpMap.end()) {
-      for (const VarDecl * Var : VarGroupForVD->second) {
-        if (Var == VD.first) {
-          continue;
-        }
+  // The map that maps each variable `v` to fix-its for the whole group where
+  // `v` is in:
+  std::map<const VarDecl *, FixItList> FinalFixItsForVariable{
+      FixItsForVariable};
 
-        FixItList GroupFix;
-        if (FixItsForVariable.find(Var) == FixItsForVariable.end()) {
-          GroupFix = fixVariable(Var, ReplacementTypeForVD, D, Tracker,
-                                 Var->getASTContext(), Handler);
-        } else {
-          GroupFix = FixItsForVariable[Var];
-        }
+  for (auto &[Var, Ignore] : FixItsForVariable) {
+    const auto VarGroupForVD = VarGrpMgr.getGroupOfVar(Var);
 
-        for (auto Fix : GroupFix) {
-          FixItsForVariable[VD.first].push_back(Fix);
-        }
-      }
+    for (const VarDecl *GrpMate : VarGroupForVD) {
+      if (Var == GrpMate)
+        continue;
+      if (FixItsForVariable.count(GrpMate))
+        FinalFixItsForVariable[Var].insert(FinalFixItsForVariable[Var].end(),
+                                           FixItsForVariable[GrpMate].begin(),
+                                           FixItsForVariable[GrpMate].end());
     }
   }
-  return FixItsForVariable;
+  return FinalFixItsForVariable;
 }
 
 
@@ -2329,6 +2328,24 @@ getNaiveStrategy(const llvm::SmallVectorImpl<const VarDecl *> &UnsafeVars) {
   return S;
 }
 
+//  Manages variable groups:
+class VariableGroupsManagerImpl : public VariableGroupsManager {
+  const std::vector<VarGrpTy> Groups;
+  const std::map<const VarDecl *, unsigned> &VarGrpMap;
+
+public:
+  VariableGroupsManagerImpl(
+      const std::vector<VarGrpTy> &Groups,
+      const std::map<const VarDecl *, unsigned> &VarGrpMap)
+      : Groups(Groups), VarGrpMap(VarGrpMap) {}
+
+  VarGrpRef getGroupOfVar(const VarDecl *Var) const override {
+    auto I = VarGrpMap.find(Var);
+    assert(I != VarGrpMap.end());
+    return Groups[I->second];
+  }
+};
+
 void clang::checkUnsafeBufferUsage(const Decl *D,
                                    UnsafeBufferUsageHandler &Handler,
                                    bool EmitSuggestions) {
@@ -2408,7 +2425,6 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
   FixablesForAllVars = groupFixablesByVar(std::move(FixableGadgets));
 
   std::map<const VarDecl *, FixItList> FixItsForVariableGroup;
-  DefMapTy VariableGroupsMap{};
 
   // Filter out non-local vars and vars with unclaimed DeclRefExpr-s.
   for (auto it = FixablesForAllVars.byVar.cbegin();
@@ -2451,7 +2467,7 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
     UnsafeVars.push_back(VD);
 
   // 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{};
   DepMapTy PtrAssignmentGraph{};
 
@@ -2460,7 +2476,7 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
       std::optional<std::pair<const VarDecl *, const VarDecl *>> ImplPair =
                                   fixable->getStrategyImplications();
       if (ImplPair) {
-        std::pair<const VarDecl *, const VarDecl *> Impl = ImplPair.value();
+        std::pair<const VarDecl *, const VarDecl *> Impl = std::move(*ImplPair);
         PtrAssignmentGraph[Impl.first].insert(Impl.second);
       }
     }
@@ -2505,14 +2521,21 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
     }
   }
 
+  // `Groups` stores the set of Connected Components in the graph.
+  std::vector<VarGrpTy> Groups;
+  // `VarGrpMap` maps variables that need fix to the groups (indexes) that the
+  // variables belong to.  Group indexes refer to the elements in `Groups`.
+  // `VarGrpMap` is complete in that every variable that needs fix is in it.
+  std::map<const VarDecl *, unsigned> VarGrpMap;
+
   // Group Connected Components for Unsafe Vars
   // (Dependencies based on pointer assignments)
   std::set<const VarDecl *> VisitedVars{};
   for (const auto &[Var, ignore] : UnsafeOps.byVar) {
     if (VisitedVars.find(Var) == VisitedVars.end()) {
-      std::vector<const VarDecl *> VarGroup{};
-
+      VarGrpTy &VarGroup = Groups.emplace_back();
       std::queue<const VarDecl*> Queue{};
+
       Queue.push(Var);
       while(!Queue.empty()) {
         const VarDecl* CurrentVar = Queue.front();
@@ -2526,10 +2549,10 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
           }
         }
       }
-      for (const VarDecl * V : VarGroup) {
-        if (UnsafeOps.byVar.find(V) != UnsafeOps.byVar.end()) {
-          VariableGroupsMap[V] = VarGroup;
-        }
+      unsigned GrpIdx = Groups.size() - 1;
+
+      for (const VarDecl *V : VarGroup) {
+        VarGrpMap[V] = GrpIdx;
       }
     }
   }
@@ -2563,20 +2586,19 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
   }
 
   Strategy NaiveStrategy = getNaiveStrategy(UnsafeVars);
+  VariableGroupsManagerImpl VarGrpMgr(Groups, VarGrpMap);
 
   FixItsForVariableGroup =
       getFixIts(FixablesForAllVars, NaiveStrategy, D->getASTContext(), D,
-                Tracker, Handler, VariableGroupsMap);
-
-  // FIXME Detect overlapping FixIts.
+                Tracker, Handler, VarGrpMgr);
 
   for (const auto &G : UnsafeOps.noVar) {
     Handler.handleUnsafeOperation(G->getBaseStmt(), /*IsRelatedToDecl=*/false);
   }
 
   for (const auto &[VD, WarningGadgets] : UnsafeOps.byVar) {
     auto FixItsIt = FixItsForVariableGroup.find(VD);
-    Handler.handleUnsafeVariableGroup(VD, VariableGroupsMap,
+    Handler.handleUnsafeVariableGroup(VD, VarGrpMgr,
                                       FixItsIt != FixItsForVariableGroup.end()
                                       ? std::move(FixItsIt->second)
                                       : FixItList{});

clang/lib/Sema/AnalysisBasedWarnings.cpp

@@ -2175,6 +2175,41 @@ class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler {
   Sema &S;
   bool SuggestSuggestions;  // Recommend -fsafe-buffer-usage-suggestions?
 
+  // Lists as a string the names of variables in `VarGroupForVD` except for `VD`
+  // itself:
+  std::string listVariableGroupAsString(
+      const VarDecl *VD, const ArrayRef<const VarDecl *> &VarGroupForVD) const {
+    if (VarGroupForVD.size() <= 1)
+      return "";
+
+    std::vector<StringRef> VarNames;
+    auto PutInQuotes = [](StringRef S) -> std::string {
+      return "'" + S.str() + "'";
+    };
+
+    for (auto *V : VarGroupForVD) {
+      if (V == VD)
+        continue;
+      VarNames.push_back(V->getName());
+    }
+    if (VarNames.size() == 1) {
+      return PutInQuotes(VarNames[0]);
+    }
+    if (VarNames.size() == 2) {
+      return PutInQuotes(VarNames[0]) + " and " + PutInQuotes(VarNames[1]);
+    }
+    assert(VarGroupForVD.size() > 3);
+    const unsigned N = VarNames.size() -
+                       2; // need to print the last two names as "..., X, and Y"
+    std::string AllVars = "";
+
+    for (unsigned I = 0; I < N; ++I)
+      AllVars.append(PutInQuotes(VarNames[I]) + ", ");
+    AllVars.append(PutInQuotes(VarNames[N]) + ", and " +
+                   PutInQuotes(VarNames[N + 1]));
+    return AllVars;
+  }
+
 public:
   UnsafeBufferUsageReporter(Sema &S, bool SuggestSuggestions)
     : S(S), SuggestSuggestions(SuggestSuggestions) {}
@@ -2231,62 +2266,25 @@ class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler {
   }
 
   void handleUnsafeVariableGroup(const VarDecl *Variable,
-                                 const DefMapTy &VarGrpMap,
-                             FixItList &&Fixes) override {
+                                 const VariableGroupsManager &VarGrpMgr,
+                                 FixItList &&Fixes) override {
     assert(!SuggestSuggestions &&
            "Unsafe buffer usage fixits displayed without suggestions!");
     S.Diag(Variable->getLocation(), diag::warn_unsafe_buffer_variable)
         << Variable << (Variable->getType()->isPointerType() ? 0 : 1)
         << Variable->getSourceRange();
     if (!Fixes.empty()) {
-      const auto VarGroupForVD = VarGrpMap.find(Variable)->second;
+      const auto VarGroupForVD = VarGrpMgr.getGroupOfVar(Variable);
       unsigned FixItStrategy = 0; // For now we only have 'std::span' strategy
       const auto &FD = S.Diag(Variable->getLocation(),
                               diag::note_unsafe_buffer_variable_fixit_group);
 
       FD << Variable << FixItStrategy;
-      std::string AllVars = "";
-      if (VarGroupForVD.size() > 1) {
-        if (VarGroupForVD.size() == 2) {
-          if (VarGroupForVD[0] == Variable) {
-            AllVars.append("'" + VarGroupForVD[1]->getName().str() + "'");
-          } else {
-            AllVars.append("'" + VarGroupForVD[0]->getName().str() + "'");
-          }
-        } else {
-          bool first = false;
-          if (VarGroupForVD.size() == 3) {
-            for (const VarDecl * V : VarGroupForVD) {
-              if (V == Variable) {
-                continue;
-              }
-              if (!first) {
-                first = true;
-                AllVars.append("'" + V->getName().str() + "'" + " and ");
-              } else {
-                AllVars.append("'" + V->getName().str() + "'");
-              }
-            }
-          } else {
-            for (const VarDecl * V : VarGroupForVD) {
-              if (V == Variable) {
-                continue;
-              }
-              if (VarGroupForVD.back() != V) {
-                AllVars.append("'" + V->getName().str() + "'" + ", ");
-              } else {
-                AllVars.append("and '" + V->getName().str() + "'");
-              }
-            }
-          }
-        }
-        FD << AllVars << 1;
-      } else {
-        FD << "" << 0;
-      }
-
-      for (const auto &F : Fixes)
+      FD << listVariableGroupAsString(Variable, VarGroupForVD)
+         << (VarGroupForVD.size() > 1);
+      for (const auto &F : Fixes) {
         FD << F;
+      }
     }
 
 #ifndef NDEBUG