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

This reverts commit 8437847.
There is a build failure caused by this commit.

Author: ziqingluo-90

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

@@ -20,19 +20,7 @@
 
 namespace clang {
 
-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;
-};
+using DefMapTy = llvm::DenseMap<const VarDecl *, std::vector<const VarDecl *>>;
 
 /// The interface that lets the caller handle unsafe buffer usage analysis
 /// results by overriding this class's handle... methods.
@@ -65,7 +53,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 VariableGroupsManager &VarGrpMgr,
+                                         const DefMapTy &VarGrpMap,
                                          FixItList &&Fixes) = 0;
 
 #ifndef NDEBUG

clang/lib/Analysis/UnsafeBufferUsage.cpp

@@ -1176,11 +1176,7 @@ groupWarningGadgetsByVar(const WarningGadgetList &AllUnsafeOperations) {
 }
 
 struct FixableGadgetSets {
-  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;
+  std::map<const VarDecl *, std::set<const FixableGadget *>> byVar;
 };
 
 static FixableGadgetSets
@@ -1386,7 +1382,7 @@ static std::optional<StringRef> getRangeText(SourceRange SR,
                                              const SourceManager &SM,
                                              const LangOptions &LangOpts) {
   bool Invalid = false;
-  CharSourceRange CSR = CharSourceRange::getCharRange(SR);
+  CharSourceRange CSR = CharSourceRange::getCharRange(SR.getBegin(), SR.getEnd());
   StringRef Text = Lexer::getSourceText(CSR, SM, LangOpts, &Invalid);
 
   if (!Invalid)
@@ -2229,7 +2225,7 @@ getFixIts(FixableGadgetSets &FixablesForAllVars, const Strategy &S,
 	  ASTContext &Ctx,
           /* The function decl under analysis */ const Decl *D,
           const DeclUseTracker &Tracker, UnsafeBufferUsageHandler &Handler,
-          const VariableGroupsManager &VarGrpMgr) {
+          const DefMapTy &VarGrpMap) {
   std::map<const VarDecl *, FixItList> FixItsForVariable;
   for (const auto &[VD, Fixables] : FixablesForAllVars.byVar) {
     FixItsForVariable[VD] =
@@ -2265,10 +2261,9 @@ getFixIts(FixableGadgetSets &FixablesForAllVars, const Strategy &S,
       continue;
     }
 
-    
-   {
-      const auto VarGroupForVD = VarGrpMgr.getGroupOfVar(VD);
-      for (const VarDecl * V : VarGroupForVD) {
+    const auto VarGroupForVD = VarGrpMap.find(VD);
+    if (VarGroupForVD != VarGrpMap.end()) {
+      for (const VarDecl * V : VarGroupForVD->second) {
         if (V == VD) {
           continue;
         }
@@ -2280,7 +2275,7 @@ getFixIts(FixableGadgetSets &FixablesForAllVars, const Strategy &S,
 
       if (ImpossibleToFix) {
         FixItsForVariable.erase(VD);
-        for (const VarDecl * V : VarGroupForVD) {
+        for (const VarDecl * V : VarGroupForVD->second) {
           FixItsForVariable.erase(V);
         }
         continue;
@@ -2298,24 +2293,30 @@ getFixIts(FixableGadgetSets &FixablesForAllVars, const Strategy &S,
     }
   }
 
-  // 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};
+  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;
+        }
 
-  for (auto &[Var, Ignore] : FixItsForVariable) {
-    const auto VarGroupForVD = VarGrpMgr.getGroupOfVar(Var);
+        FixItList GroupFix;
+        if (FixItsForVariable.find(Var) == FixItsForVariable.end()) {
+          GroupFix = fixVariable(Var, ReplacementTypeForVD, D, Tracker,
+                                 Var->getASTContext(), Handler);
+        } else {
+          GroupFix = FixItsForVariable[Var];
+        }
 
-    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());
+        for (auto Fix : GroupFix) {
+          FixItsForVariable[VD.first].push_back(Fix);
+        }
+      }
     }
   }
-  return FinalFixItsForVariable;
+  return FixItsForVariable;
 }
 
 
@@ -2328,24 +2329,6 @@ 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) {
@@ -2425,6 +2408,7 @@ 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();
@@ -2467,7 +2451,7 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
     UnsafeVars.push_back(VD);
 
   // Fixpoint iteration for pointer assignments
-  using DepMapTy = DenseMap<const VarDecl *, llvm::SetVector<const VarDecl *>>;
+  using DepMapTy = DenseMap<const VarDecl *, std::set<const VarDecl *>>;
   DepMapTy DependenciesMap{};
   DepMapTy PtrAssignmentGraph{};
 
@@ -2476,7 +2460,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 = std::move(*ImplPair);
+        std::pair<const VarDecl *, const VarDecl *> Impl = ImplPair.value();
         PtrAssignmentGraph[Impl.first].insert(Impl.second);
       }
     }
@@ -2521,21 +2505,14 @@ 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()) {
-      VarGrpTy &VarGroup = Groups.emplace_back();
-      std::queue<const VarDecl*> Queue{};
+      std::vector<const VarDecl *> VarGroup{};
 
+      std::queue<const VarDecl*> Queue{};
       Queue.push(Var);
       while(!Queue.empty()) {
         const VarDecl* CurrentVar = Queue.front();
@@ -2549,10 +2526,10 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
           }
         }
       }
-      unsigned GrpIdx = Groups.size() - 1;
-
-      for (const VarDecl *V : VarGroup) {
-        VarGrpMap[V] = GrpIdx;
+      for (const VarDecl * V : VarGroup) {
+        if (UnsafeOps.byVar.find(V) != UnsafeOps.byVar.end()) {
+          VariableGroupsMap[V] = VarGroup;
+        }
       }
     }
   }
@@ -2586,19 +2563,20 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
   }
 
   Strategy NaiveStrategy = getNaiveStrategy(UnsafeVars);
-  VariableGroupsManagerImpl VarGrpMgr(Groups, VarGrpMap);
 
   FixItsForVariableGroup =
       getFixIts(FixablesForAllVars, NaiveStrategy, D->getASTContext(), D,
-                Tracker, Handler, VarGrpMgr);
+                Tracker, Handler, VariableGroupsMap);
+
+  // FIXME Detect overlapping FixIts.
 
   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, VarGrpMgr,
+    Handler.handleUnsafeVariableGroup(VD, VariableGroupsMap,
                                       FixItsIt != FixItsForVariableGroup.end()
                                       ? std::move(FixItsIt->second)
                                       : FixItList{});

clang/lib/Sema/AnalysisBasedWarnings.cpp

@@ -2175,41 +2175,6 @@ 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) {}
@@ -2266,25 +2231,62 @@ class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler {
   }
 
   void handleUnsafeVariableGroup(const VarDecl *Variable,
-                                 const VariableGroupsManager &VarGrpMgr,
-                                 FixItList &&Fixes) override {
+                                 const DefMapTy &VarGrpMap,
+                             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 = VarGrpMgr.getGroupOfVar(Variable);
+      const auto VarGroupForVD = VarGrpMap.find(Variable)->second;
       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;
-      FD << listVariableGroupAsString(Variable, VarGroupForVD)
-         << (VarGroupForVD.size() > 1);
-      for (const auto &F : Fixes) {
-        FD << F;
+      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 << F;
     }
 
 #ifndef NDEBUG