[clang] acc8a33 - [-Wunsafe-buffer-usage][NFC] Refactor `getFixIts`---where fix-its are generated
via cfe-commits
cfe-commits at lists.llvm.org
Fri Aug 18 17:44:45 PDT 2023
Author: ziqingluo-90
Date: 2023-08-18T17:44:22-07:00
New Revision: acc8a33b257f8176fae1bae929ff9da7ed2cbad5
URL: https://github.com/llvm/llvm-project/commit/acc8a33b257f8176fae1bae929ff9da7ed2cbad5
DIFF: https://github.com/llvm/llvm-project/commit/acc8a33b257f8176fae1bae929ff9da7ed2cbad5.diff
LOG: [-Wunsafe-buffer-usage][NFC] Refactor `getFixIts`---where fix-its are generated
Refactor the getFixIts function for better readability.
Reviewed by: NoQ (Artem Dergachev), t-rasmud (Rashmi Mudduluru)
Differential revision: https://reviews.llvm.org/D156762
Added:
Modified:
clang/lib/Analysis/UnsafeBufferUsage.cpp
Removed:
################################################################################
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 4638f7422320b9..21d3f6f2faaa68 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -2212,16 +2212,30 @@ static bool overlapWithMacro(const FixItList &FixIts) {
});
}
-static bool impossibleToFixForVar(const FixableGadgetSets &FixablesForAllVars,
- const Strategy &S,
- const VarDecl * Var) {
- for (const auto &F : FixablesForAllVars.byVar.find(Var)->second) {
- std::optional<FixItList> Fixits = F->getFixits(S);
- if (!Fixits) {
- return true;
+// Erases variables in `FixItsForVariable`, if such a variable has an unfixable
+// group mate. A variable `v` is unfixable iff `FixItsForVariable` does not
+// contain `v`.
+static void eraseVarsForUnfixableGroupMates(
+ std::map<const VarDecl *, FixItList> &FixItsForVariable,
+ const VariableGroupsManager &VarGrpMgr) {
+ // Variables will be removed from `FixItsForVariable`:
+ SmallVector<const VarDecl *, 8> ToErase;
+
+ for (auto [VD, Ignore] : FixItsForVariable) {
+ VarGrpRef Grp = VarGrpMgr.getGroupOfVar(VD);
+ if (llvm::any_of(Grp,
+ [&FixItsForVariable](const VarDecl *GrpMember) -> bool {
+ return FixItsForVariable.find(GrpMember) ==
+ FixItsForVariable.end();
+ })) {
+ // At least one group member cannot be fixed, so we have to erase the
+ // whole group:
+ for (const VarDecl *Member : Grp)
+ ToErase.push_back(Member);
}
}
- return false;
+ for (auto *VarToErase : ToErase)
+ FixItsForVariable.erase(VarToErase);
}
static std::map<const VarDecl *, FixItList>
@@ -2230,7 +2244,14 @@ getFixIts(FixableGadgetSets &FixablesForAllVars, const Strategy &S,
/* The function decl under analysis */ const Decl *D,
const DeclUseTracker &Tracker, UnsafeBufferUsageHandler &Handler,
const VariableGroupsManager &VarGrpMgr) {
+ // `FixItsForVariable` will map each variable to a set of fix-its directly
+ // associated to the variable itself. Fix-its of distinct variables in
+ // `FixItsForVariable` are disjoint.
std::map<const VarDecl *, FixItList> FixItsForVariable;
+
+ // Populate `FixItsForVariable` with fix-its directly associated with each
+ // variable. Fix-its directly associated to a variable 'v' are the ones
+ // produced by the `FixableGadget`s whose claimed variable is 'v'.
for (const auto &[VD, Fixables] : FixablesForAllVars.byVar) {
FixItsForVariable[VD] =
fixVariable(VD, S.lookup(VD), D, Tracker, Ctx, Handler);
@@ -2240,64 +2261,36 @@ getFixIts(FixableGadgetSets &FixablesForAllVars, const Strategy &S,
FixItsForVariable.erase(VD);
continue;
}
- bool ImpossibleToFix = false;
- llvm::SmallVector<FixItHint, 16> FixItsForVD;
for (const auto &F : Fixables) {
std::optional<FixItList> Fixits = F->getFixits(S);
- if (!Fixits) {
-#ifndef NDEBUG
- Handler.addDebugNoteForVar(
- VD, F->getBaseStmt()->getBeginLoc(),
- ("gadget '" + F->getDebugName() + "' refused to produce a fix")
- .str());
-#endif
- ImpossibleToFix = true;
- break;
- } else {
- const FixItList CorrectFixes = Fixits.value();
- FixItsForVD.insert(FixItsForVD.end(), CorrectFixes.begin(),
- CorrectFixes.end());
- }
- }
- if (ImpossibleToFix) {
- FixItsForVariable.erase(VD);
- continue;
- }
-
-
- {
- const auto VarGroupForVD = VarGrpMgr.getGroupOfVar(VD);
- for (const VarDecl * V : VarGroupForVD) {
- if (V == VD) {
- continue;
- }
- if (impossibleToFixForVar(FixablesForAllVars, S, V)) {
- ImpossibleToFix = true;
- break;
- }
- }
-
- if (ImpossibleToFix) {
- FixItsForVariable.erase(VD);
- for (const VarDecl * V : VarGroupForVD) {
- FixItsForVariable.erase(V);
- }
+ if (Fixits) {
+ FixItsForVariable[VD].insert(FixItsForVariable[VD].end(),
+ Fixits->begin(), Fixits->end());
continue;
}
- }
- FixItsForVariable[VD].insert(FixItsForVariable[VD].end(),
- FixItsForVD.begin(), FixItsForVD.end());
-
- // Fix-it shall not overlap with macros or/and templates:
- if (overlapWithMacro(FixItsForVariable[VD]) ||
- clang::internal::anyConflict(FixItsForVariable[VD],
- Ctx.getSourceManager())) {
+#ifndef NDEBUG
+ Handler.addDebugNoteForVar(
+ VD, F->getBaseStmt()->getBeginLoc(),
+ ("gadget '" + F->getDebugName() + "' refused to produce a fix")
+ .str());
+#endif
FixItsForVariable.erase(VD);
- continue;
+ break;
}
}
+ // `FixItsForVariable` now contains only variables that can be
+ // fixed. A variable can be fixed if its' declaration and all Fixables
+ // associated to it can all be fixed.
+
+ // To further remove from `FixItsForVariable` variables whose group mates
+ // cannot be fixed...
+ eraseVarsForUnfixableGroupMates(FixItsForVariable, VarGrpMgr);
+ // Now `FixItsForVariable` gets further reduced: a variable is in
+ // `FixItsForVariable` iff it can be fixed and all its group mates can be
+ // fixed.
+
// The map that maps each variable `v` to fix-its for the whole group where
// `v` is in:
std::map<const VarDecl *, FixItList> FinalFixItsForVariable{
@@ -2315,10 +2308,20 @@ getFixIts(FixableGadgetSets &FixablesForAllVars, const Strategy &S,
FixItsForVariable[GrpMate].end());
}
}
+ // Fix-its that will be applied in one step shall NOT:
+ // 1. overlap with macros or/and templates; or
+ // 2. conflict with each other.
+ // Otherwise, the fix-its will be dropped.
+ for (auto Iter = FinalFixItsForVariable.begin();
+ Iter != FinalFixItsForVariable.end();)
+ if (overlapWithMacro(Iter->second) ||
+ clang::internal::anyConflict(Iter->second, Ctx.getSourceManager())) {
+ Iter = FinalFixItsForVariable.erase(Iter);
+ } else
+ Iter++;
return FinalFixItsForVariable;
}
-
static Strategy
getNaiveStrategy(const llvm::SmallVectorImpl<const VarDecl *> &UnsafeVars) {
Strategy S;
@@ -2356,6 +2359,7 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
assert(D && D->getBody());
// We do not want to visit a Lambda expression defined inside a method independently.
// Instead, it should be visited along with the outer method.
+ // FIXME: do we want to do the same thing for `BlockDecl`s?
if (const auto *fd = dyn_cast<CXXMethodDecl>(D)) {
if (fd->getParent()->isLambda() && fd->getParent()->isLocalClass())
return;
More information about the cfe-commits
mailing list