[clang] 0456acb - [clang/Sema] Follow-up for fix of non-deterministic order of `-Wunused-variable` diagnostic
Argyrios Kyrtzidis via cfe-commits
cfe-commits at lists.llvm.org
Tue Oct 11 13:43:56 PDT 2022
Author: Argyrios Kyrtzidis
Date: 2022-10-11T13:39:26-07:00
New Revision: 0456acbfb942f127359a8defd1b4f1f44420df3e
URL: https://github.com/llvm/llvm-project/commit/0456acbfb942f127359a8defd1b4f1f44420df3e
DIFF: https://github.com/llvm/llvm-project/commit/0456acbfb942f127359a8defd1b4f1f44420df3e.diff
LOG: [clang/Sema] Follow-up for fix of non-deterministic order of `-Wunused-variable` diagnostic
Commit `371883f46dc23f8464cbf578e2d12a4f92e61917` caused a noticeable compile-time regression (about 0.4% geomean at -O0):
http://llvm-compile-time-tracker.com/compare.php?from=92233159035d1b50face95d886901cf99035bd99&to=371883f46dc23f8464cbf578e2d12a4f92e61917&stat=instructions
To address this switch `Scope::DeclSetTy` back to a `SmallPtrSet` and change `Sema::ActOnPopScope` to explicitly order the diagnostics that this function emits.
Differential Revision: https://reviews.llvm.org/D135490
Added:
Modified:
clang/include/clang/Sema/Scope.h
clang/include/clang/Sema/Sema.h
clang/lib/Sema/SemaDecl.cpp
Removed:
################################################################################
diff --git a/clang/include/clang/Sema/Scope.h b/clang/include/clang/Sema/Scope.h
index 04ab416bf7352..d2acf253552dc 100644
--- a/clang/include/clang/Sema/Scope.h
+++ b/clang/include/clang/Sema/Scope.h
@@ -16,7 +16,6 @@
#include "clang/AST/Decl.h"
#include "clang/Basic/Diagnostic.h"
#include "llvm/ADT/PointerIntPair.h"
-#include "llvm/ADT/SetVector.h"
#include "llvm/ADT/SmallPtrSet.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/iterator_range.h"
@@ -197,7 +196,7 @@ class Scope {
/// popped, these declarations are removed from the IdentifierTable's notion
/// of current declaration. It is up to the current Action implementation to
/// implement these semantics.
- using DeclSetTy = llvm::SmallSetVector<Decl *, 32>;
+ using DeclSetTy = llvm::SmallPtrSet<Decl *, 32>;
DeclSetTy DeclsInScope;
/// The DeclContext with which this scope is associated. For
@@ -322,7 +321,7 @@ class Scope {
DeclsInScope.insert(D);
}
- void RemoveDecl(Decl *D) { DeclsInScope.remove(D); }
+ void RemoveDecl(Decl *D) { DeclsInScope.erase(D); }
void incrementMSManglingNumber() {
if (Scope *MSLMP = getMSLastManglingParent()) {
@@ -350,9 +349,7 @@ class Scope {
/// isDeclScope - Return true if this is the scope that the specified decl is
/// declared in.
- bool isDeclScope(const Decl *D) const {
- return DeclsInScope.contains(const_cast<Decl *>(D));
- }
+ bool isDeclScope(const Decl *D) const { return DeclsInScope.contains(D); }
/// Get the entity corresponding to this scope.
DeclContext *getEntity() const {
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index a6acdf6b49526..405f84f0b5854 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -5221,15 +5221,21 @@ class Sema final {
/// of it.
void MarkUnusedFileScopedDecl(const DeclaratorDecl *D);
+ typedef llvm::function_ref<void(SourceLocation Loc, PartialDiagnostic PD)>
+ DiagReceiverTy;
+
/// DiagnoseUnusedExprResult - If the statement passed in is an expression
/// whose result is unused, warn.
void DiagnoseUnusedExprResult(const Stmt *S, unsigned DiagID);
void DiagnoseUnusedNestedTypedefs(const RecordDecl *D);
+ void DiagnoseUnusedNestedTypedefs(const RecordDecl *D,
+ DiagReceiverTy DiagReceiver);
void DiagnoseUnusedDecl(const NamedDecl *ND);
+ void DiagnoseUnusedDecl(const NamedDecl *ND, DiagReceiverTy DiagReceiver);
/// If VD is set but not otherwise used, diagnose, for a parameter or a
/// variable.
- void DiagnoseUnusedButSetDecl(const VarDecl *VD);
+ void DiagnoseUnusedButSetDecl(const VarDecl *VD, DiagReceiverTy DiagReceiver);
/// Emit \p DiagID if statement located on \p StmtLoc has a suspicious null
/// statement as a \p Body, and it is located on the same line.
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 1bf959a35178a..5cc3db63a1996 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -2091,20 +2091,31 @@ static void GenerateFixForUnusedDecl(const NamedDecl *D, ASTContext &Ctx,
}
void Sema::DiagnoseUnusedNestedTypedefs(const RecordDecl *D) {
+ DiagnoseUnusedNestedTypedefs(
+ D, [this](SourceLocation Loc, PartialDiagnostic PD) { Diag(Loc, PD); });
+}
+
+void Sema::DiagnoseUnusedNestedTypedefs(const RecordDecl *D,
+ DiagReceiverTy DiagReceiver) {
if (D->getTypeForDecl()->isDependentType())
return;
for (auto *TmpD : D->decls()) {
if (const auto *T = dyn_cast<TypedefNameDecl>(TmpD))
- DiagnoseUnusedDecl(T);
+ DiagnoseUnusedDecl(T, DiagReceiver);
else if(const auto *R = dyn_cast<RecordDecl>(TmpD))
- DiagnoseUnusedNestedTypedefs(R);
+ DiagnoseUnusedNestedTypedefs(R, DiagReceiver);
}
}
+void Sema::DiagnoseUnusedDecl(const NamedDecl *D) {
+ DiagnoseUnusedDecl(
+ D, [this](SourceLocation Loc, PartialDiagnostic PD) { Diag(Loc, PD); });
+}
+
/// DiagnoseUnusedDecl - Emit warnings about declarations that are not used
/// unless they are marked attr(unused).
-void Sema::DiagnoseUnusedDecl(const NamedDecl *D) {
+void Sema::DiagnoseUnusedDecl(const NamedDecl *D, DiagReceiverTy DiagReceiver) {
if (!ShouldDiagnoseUnusedDecl(D))
return;
@@ -2126,10 +2137,11 @@ void Sema::DiagnoseUnusedDecl(const NamedDecl *D) {
else
DiagID = diag::warn_unused_variable;
- Diag(D->getLocation(), DiagID) << D << Hint;
+ DiagReceiver(D->getLocation(), PDiag(DiagID) << D << Hint);
}
-void Sema::DiagnoseUnusedButSetDecl(const VarDecl *VD) {
+void Sema::DiagnoseUnusedButSetDecl(const VarDecl *VD,
+ DiagReceiverTy DiagReceiver) {
// If it's not referenced, it can't be set. If it has the Cleanup attribute,
// it's not really unused.
if (!VD->isReferenced() || !VD->getDeclName() || VD->hasAttr<UnusedAttr>() ||
@@ -2175,10 +2187,11 @@ void Sema::DiagnoseUnusedButSetDecl(const VarDecl *VD) {
return;
unsigned DiagID = isa<ParmVarDecl>(VD) ? diag::warn_unused_but_set_parameter
: diag::warn_unused_but_set_variable;
- Diag(VD->getLocation(), DiagID) << VD;
+ DiagReceiver(VD->getLocation(), PDiag(DiagID) << VD);
}
-static void CheckPoppedLabel(LabelDecl *L, Sema &S) {
+static void CheckPoppedLabel(LabelDecl *L, Sema &S,
+ Sema::DiagReceiverTy DiagReceiver) {
// Verify that we have no forward references left. If so, there was a goto
// or address of a label taken, but no definition of it. Label fwd
// definitions are indicated with a null substmt which is also not a resolved
@@ -2189,7 +2202,8 @@ static void CheckPoppedLabel(LabelDecl *L, Sema &S) {
else
Diagnose = L->getStmt() == nullptr;
if (Diagnose)
- S.Diag(L->getLocation(), diag::err_undeclared_label_use) << L;
+ DiagReceiver(L->getLocation(), S.PDiag(diag::err_undeclared_label_use)
+ << L);
}
void Sema::ActOnPopScope(SourceLocation Loc, Scope *S) {
@@ -2199,6 +2213,24 @@ void Sema::ActOnPopScope(SourceLocation Loc, Scope *S) {
assert((S->getFlags() & (Scope::DeclScope | Scope::TemplateParamScope)) &&
"Scope shouldn't contain decls!");
+ /// We visit the decls in non-deterministic order, but we want diagnostics
+ /// emitted in deterministic order. Collect any diagnostic that may be emitted
+ /// and sort the diagnostics before emitting them, after we visited all decls.
+ struct LocAndDiag {
+ SourceLocation Loc;
+ Optional<SourceLocation> PreviousDeclLoc;
+ PartialDiagnostic PD;
+ };
+ SmallVector<LocAndDiag, 16> DeclDiags;
+ auto addDiag = [&DeclDiags](SourceLocation Loc, PartialDiagnostic PD) {
+ DeclDiags.push_back(LocAndDiag{Loc, None, std::move(PD)});
+ };
+ auto addDiagWithPrev = [&DeclDiags](SourceLocation Loc,
+ SourceLocation PreviousDeclLoc,
+ PartialDiagnostic PD) {
+ DeclDiags.push_back(LocAndDiag{Loc, PreviousDeclLoc, std::move(PD)});
+ };
+
for (auto *TmpD : S->decls()) {
assert(TmpD && "This decl didn't get pushed??");
@@ -2207,11 +2239,11 @@ void Sema::ActOnPopScope(SourceLocation Loc, Scope *S) {
// Diagnose unused variables in this scope.
if (!S->hasUnrecoverableErrorOccurred()) {
- DiagnoseUnusedDecl(D);
+ DiagnoseUnusedDecl(D, addDiag);
if (const auto *RD = dyn_cast<RecordDecl>(D))
- DiagnoseUnusedNestedTypedefs(RD);
+ DiagnoseUnusedNestedTypedefs(RD, addDiag);
if (VarDecl *VD = dyn_cast<VarDecl>(D)) {
- DiagnoseUnusedButSetDecl(VD);
+ DiagnoseUnusedButSetDecl(VD, addDiag);
RefsMinusAssignments.erase(VD);
}
}
@@ -2220,7 +2252,7 @@ void Sema::ActOnPopScope(SourceLocation Loc, Scope *S) {
// If this was a forward reference to a label, verify it was defined.
if (LabelDecl *LD = dyn_cast<LabelDecl>(D))
- CheckPoppedLabel(LD, *this);
+ CheckPoppedLabel(LD, *this, addDiag);
// Remove this name from our lexical scope, and warn on it if we haven't
// already.
@@ -2228,13 +2260,27 @@ void Sema::ActOnPopScope(SourceLocation Loc, Scope *S) {
auto ShadowI = ShadowingDecls.find(D);
if (ShadowI != ShadowingDecls.end()) {
if (const auto *FD = dyn_cast<FieldDecl>(ShadowI->second)) {
- Diag(D->getLocation(), diag::warn_ctor_parm_shadows_field)
- << D << FD << FD->getParent();
- Diag(FD->getLocation(), diag::note_previous_declaration);
+ addDiagWithPrev(D->getLocation(), FD->getLocation(),
+ PDiag(diag::warn_ctor_parm_shadows_field)
+ << D << FD << FD->getParent());
}
ShadowingDecls.erase(ShadowI);
}
}
+
+ llvm::sort(DeclDiags,
+ [](const LocAndDiag &LHS, const LocAndDiag &RHS) -> bool {
+ // The particular order for diagnostics is not important, as long
+ // as the order is deterministic. Using the raw location is going
+ // to generally be in source order unless there are macro
+ // expansions involved.
+ return LHS.Loc.getRawEncoding() < RHS.Loc.getRawEncoding();
+ });
+ for (const LocAndDiag &D : DeclDiags) {
+ Diag(D.Loc, D.PD);
+ if (D.PreviousDeclLoc)
+ Diag(*D.PreviousDeclLoc, diag::note_previous_declaration);
+ }
}
/// Look for an Objective-C class in the translation unit.
More information about the cfe-commits
mailing list