[Clang][NFCI] Remove records of unsatisfied atomic expressions in ConstraintSatisfaction

[zyn0217]

This expression doesn't appear to be ever used, so let's remove it from the data structure.

Fixed some spelling issues as well.

[llvmbot]

@llvm/pr-subscribers-clang-modules

@llvm/pr-subscribers-clang

Author: Younan Zhang (zyn0217)

Changes

This expression doesn't appear to be ever used, so let's remove it from the data structure.

Fixed some spelling issues as well.

---

Full diff: https://github.com/llvm/llvm-project/pull/98654.diff

6 Files Affected:

• (modified) clang/include/clang/AST/ASTConcept.h (+4-7)
• (modified) clang/lib/AST/ASTConcept.cpp (+10-14)
• (modified) clang/lib/Sema/SemaConcept.cpp (+14-17)
• (modified) clang/lib/Sema/SemaTemplate.cpp (+2-2)
• (modified) clang/lib/Serialization/ASTReaderStmt.cpp (+4-5)
• (modified) clang/lib/Serialization/ASTWriterStmt.cpp (+3-5)

diff --git a/clang/include/clang/AST/ASTConcept.h b/clang/include/clang/AST/ASTConcept.h
index 5f9aa41d3e6cf..3c5fdf81d4b1e 100644
--- a/clang/include/clang/AST/ASTConcept.h
+++ b/clang/include/clang/AST/ASTConcept.h
@@ -53,11 +53,10 @@ class ConstraintSatisfaction : public llvm::FoldingSetNode {
   bool IsSatisfied = false;
   bool ContainsErrors = false;
 
-  /// \brief Pairs of unsatisfied atomic constraint expressions along with the
-  /// substituted constraint expr, if the template arguments could be
+  /// \brief The substituted constraint expr, if the template arguments could be
   /// substituted into them, or a diagnostic if substitution resulted in an
   /// invalid expression.
-  llvm::SmallVector<std::pair<const Expr *, Detail>, 4> Details;
+  llvm::SmallVector<Detail, 4> Details;
 
   void Profile(llvm::FoldingSetNodeID &ID, const ASTContext &C) {
     Profile(ID, C, ConstraintOwner, TemplateArgs);
@@ -69,7 +68,7 @@ class ConstraintSatisfaction : public llvm::FoldingSetNode {
 
   bool HasSubstitutionFailure() {
     for (const auto &Detail : Details)
-      if (Detail.second.dyn_cast<SubstitutionDiagnostic *>())
+      if (Detail.dyn_cast<SubstitutionDiagnostic *>())
         return true;
     return false;
   }
@@ -80,9 +79,7 @@ class ConstraintSatisfaction : public llvm::FoldingSetNode {
 /// substituted into them, or a diagnostic if substitution resulted in
 /// an invalid expression.
 using UnsatisfiedConstraintRecord =
-    std::pair<const Expr *,
-              llvm::PointerUnion<Expr *,
-                                 std::pair<SourceLocation, StringRef> *>>;
+    llvm::PointerUnion<Expr *, std::pair<SourceLocation, StringRef> *>;
 
 /// \brief The result of a constraint satisfaction check, containing the
 /// necessary information to diagnose an unsatisfied constraint.
diff --git a/clang/lib/AST/ASTConcept.cpp b/clang/lib/AST/ASTConcept.cpp
index 0387fc9f6aec2..95e7ac1a3d775 100644
--- a/clang/lib/AST/ASTConcept.cpp
+++ b/clang/lib/AST/ASTConcept.cpp
@@ -19,27 +19,23 @@
 
 using namespace clang;
 
-namespace {
-void CreatUnsatisfiedConstraintRecord(
-    const ASTContext &C, const UnsatisfiedConstraintRecord &Detail,
-    UnsatisfiedConstraintRecord *TrailingObject) {
-  if (Detail.second.is<Expr *>())
-    new (TrailingObject) UnsatisfiedConstraintRecord{
-        Detail.first,
-        UnsatisfiedConstraintRecord::second_type(Detail.second.get<Expr *>())};
+static void
+CreateUnsatisfiedConstraintRecord(const ASTContext &C,
+                                  const UnsatisfiedConstraintRecord &Detail,
+                                  UnsatisfiedConstraintRecord *TrailingObject) {
+  if (Detail.is<Expr *>())
+    new (TrailingObject) UnsatisfiedConstraintRecord(Detail.get<Expr *>());
   else {
     auto &SubstitutionDiagnostic =
-        *Detail.second.get<std::pair<SourceLocation, StringRef> *>();
+        *Detail.get<std::pair<SourceLocation, StringRef> *>();
     unsigned MessageSize = SubstitutionDiagnostic.second.size();
     char *Mem = new (C) char[MessageSize];
     memcpy(Mem, SubstitutionDiagnostic.second.data(), MessageSize);
     auto *NewSubstDiag = new (C) std::pair<SourceLocation, StringRef>(
         SubstitutionDiagnostic.first, StringRef(Mem, MessageSize));
-    new (TrailingObject) UnsatisfiedConstraintRecord{
-        Detail.first, UnsatisfiedConstraintRecord::second_type(NewSubstDiag)};
+    new (TrailingObject) UnsatisfiedConstraintRecord(NewSubstDiag);
   }
 }
-} // namespace
 
 ASTConstraintSatisfaction::ASTConstraintSatisfaction(
     const ASTContext &C, const ConstraintSatisfaction &Satisfaction)
@@ -47,7 +43,7 @@ ASTConstraintSatisfaction::ASTConstraintSatisfaction(
       IsSatisfied{Satisfaction.IsSatisfied}, ContainsErrors{
                                                  Satisfaction.ContainsErrors} {
   for (unsigned I = 0; I < NumRecords; ++I)
-    CreatUnsatisfiedConstraintRecord(
+    CreateUnsatisfiedConstraintRecord(
         C, Satisfaction.Details[I],
         getTrailingObjects<UnsatisfiedConstraintRecord>() + I);
 }
@@ -58,7 +54,7 @@ ASTConstraintSatisfaction::ASTConstraintSatisfaction(
       IsSatisfied{Satisfaction.IsSatisfied},
       ContainsErrors{Satisfaction.ContainsErrors} {
   for (unsigned I = 0; I < NumRecords; ++I)
-    CreatUnsatisfiedConstraintRecord(
+    CreateUnsatisfiedConstraintRecord(
         C, *(Satisfaction.begin() + I),
         getTrailingObjects<UnsatisfiedConstraintRecord>() + I);
 }
diff --git a/clang/lib/Sema/SemaConcept.cpp b/clang/lib/Sema/SemaConcept.cpp
index 202dd86c67f62..de8cde16dbae8 100644
--- a/clang/lib/Sema/SemaConcept.cpp
+++ b/clang/lib/Sema/SemaConcept.cpp
@@ -273,7 +273,6 @@ calculateConstraintSatisfaction(Sema &S, const Expr *ConstraintExpr,
     char *Mem = new (S.Context) char[MessageSize];
     memcpy(Mem, DiagString.c_str(), MessageSize);
     Satisfaction.Details.emplace_back(
-        ConstraintExpr,
         new (S.Context) ConstraintSatisfaction::SubstitutionDiagnostic{
             SubstitutedAtomicExpr.get()->getBeginLoc(),
             StringRef(Mem, MessageSize)});
@@ -302,8 +301,7 @@ calculateConstraintSatisfaction(Sema &S, const Expr *ConstraintExpr,
          "evaluating bool expression didn't produce int");
   Satisfaction.IsSatisfied = EvalResult.Val.getInt().getBoolValue();
   if (!Satisfaction.IsSatisfied)
-    Satisfaction.Details.emplace_back(ConstraintExpr,
-                                      SubstitutedAtomicExpr.get());
+    Satisfaction.Details.emplace_back(SubstitutedAtomicExpr.get());
 
   return SubstitutedAtomicExpr;
 }
@@ -393,9 +391,8 @@ static ExprResult calculateConstraintSatisfaction(
             char *Mem = new (S.Context) char[MessageSize];
             memcpy(Mem, DiagString.c_str(), MessageSize);
             Satisfaction.Details.emplace_back(
-                AtomicExpr,
                 new (S.Context) ConstraintSatisfaction::SubstitutionDiagnostic{
-                        SubstDiag.first, StringRef(Mem, MessageSize)});
+                    SubstDiag.first, StringRef(Mem, MessageSize)});
             Satisfaction.IsSatisfied = false;
             return ExprEmpty();
           }
@@ -1056,13 +1053,14 @@ static void diagnoseUnsatisfiedRequirement(Sema &S,
                                            concepts::NestedRequirement *Req,
                                            bool First) {
   using SubstitutionDiagnostic = std::pair<SourceLocation, StringRef>;
-  for (auto &Pair : Req->getConstraintSatisfaction()) {
-    if (auto *SubstDiag = Pair.second.dyn_cast<SubstitutionDiagnostic *>())
+  for (auto &Record : Req->getConstraintSatisfaction()) {
+    if (auto *SubstDiag = Record.dyn_cast<SubstitutionDiagnostic *>())
       S.Diag(SubstDiag->first, diag::note_nested_requirement_substitution_error)
-          << (int)First << Req->getInvalidConstraintEntity() << SubstDiag->second;
+          << (int)First << Req->getInvalidConstraintEntity()
+          << SubstDiag->second;
     else
-      diagnoseWellFormedUnsatisfiedConstraintExpr(
-          S, Pair.second.dyn_cast<Expr *>(), First);
+      diagnoseWellFormedUnsatisfiedConstraintExpr(S, Record.dyn_cast<Expr *>(),
+                                                  First);
     First = false;
   }
 }
@@ -1176,12 +1174,11 @@ static void diagnoseWellFormedUnsatisfiedConstraintExpr(Sema &S,
       << (int)First << SubstExpr;
 }
 
-template<typename SubstitutionDiagnostic>
+template <typename SubstitutionDiagnostic>
 static void diagnoseUnsatisfiedConstraintExpr(
-    Sema &S, const Expr *E,
-    const llvm::PointerUnion<Expr *, SubstitutionDiagnostic *> &Record,
+    Sema &S, const llvm::PointerUnion<Expr *, SubstitutionDiagnostic *> &Record,
     bool First = true) {
-  if (auto *Diag = Record.template dyn_cast<SubstitutionDiagnostic *>()){
+  if (auto *Diag = Record.template dyn_cast<SubstitutionDiagnostic *>()) {
     S.Diag(Diag->first, diag::note_substituted_constraint_expr_is_ill_formed)
         << Diag->second;
     return;
@@ -1196,8 +1193,8 @@ Sema::DiagnoseUnsatisfiedConstraint(const ConstraintSatisfaction& Satisfaction,
                                     bool First) {
   assert(!Satisfaction.IsSatisfied &&
          "Attempted to diagnose a satisfied constraint");
-  for (auto &Pair : Satisfaction.Details) {
-    diagnoseUnsatisfiedConstraintExpr(*this, Pair.first, Pair.second, First);
+  for (auto &Detail : Satisfaction.Details) {
+    diagnoseUnsatisfiedConstraintExpr(*this, Detail, First);
     First = false;
   }
 }
@@ -1208,7 +1205,7 @@ void Sema::DiagnoseUnsatisfiedConstraint(
   assert(!Satisfaction.IsSatisfied &&
          "Attempted to diagnose a satisfied constraint");
   for (auto &Pair : Satisfaction) {
-    diagnoseUnsatisfiedConstraintExpr(*this, Pair.first, Pair.second, First);
+    diagnoseUnsatisfiedConstraintExpr(*this, Pair, First);
     First = false;
   }
 }
diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
index 07b3f793b3a29..5b25b4a364633 100644
--- a/clang/lib/Sema/SemaTemplate.cpp
+++ b/clang/lib/Sema/SemaTemplate.cpp
@@ -4094,10 +4094,10 @@ TemplateParameterList *Sema::MatchTemplateParametersToScopeSpecifier(
 
     // C++ [temp.expl.spec]p16:
     //   In an explicit specialization declaration for a member of a class
-    //   template or a member template that ap- pears in namespace scope, the
+    //   template or a member template that appears in namespace scope, the
     //   member template and some of its enclosing class templates may remain
     //   unspecialized, except that the declaration shall not explicitly
-    //   specialize a class member template if its en- closing class templates
+    //   specialize a class member template if its enclosing class templates
     //   are not explicitly specialized as well.
     if (ParamIdx < ParamLists.size()) {
       if (ParamLists[ParamIdx]->size() == 0) {
diff --git a/clang/lib/Serialization/ASTReaderStmt.cpp b/clang/lib/Serialization/ASTReaderStmt.cpp
index e23ceffb10bfe..37f5c7f2bf20b 100644
--- a/clang/lib/Serialization/ASTReaderStmt.cpp
+++ b/clang/lib/Serialization/ASTReaderStmt.cpp
@@ -793,16 +793,15 @@ readConstraintSatisfaction(ASTRecordReader &Record) {
   if (!Satisfaction.IsSatisfied) {
     unsigned NumDetailRecords = Record.readInt();
     for (unsigned i = 0; i != NumDetailRecords; ++i) {
-      Expr *ConstraintExpr = Record.readExpr();
       if (/* IsDiagnostic */Record.readInt()) {
         SourceLocation DiagLocation = Record.readSourceLocation();
         std::string DiagMessage = Record.readString();
         Satisfaction.Details.emplace_back(
-            ConstraintExpr, new (Record.getContext())
-                                ConstraintSatisfaction::SubstitutionDiagnostic{
-                                    DiagLocation, DiagMessage});
+            new (Record.getContext())
+                ConstraintSatisfaction::SubstitutionDiagnostic(DiagLocation,
+                                                               DiagMessage));
       } else
-        Satisfaction.Details.emplace_back(ConstraintExpr, Record.readExpr());
+        Satisfaction.Details.emplace_back(Record.readExpr());
     }
   }
   return Satisfaction;
diff --git a/clang/lib/Serialization/ASTWriterStmt.cpp b/clang/lib/Serialization/ASTWriterStmt.cpp
index ea499019c9d16..d36f43fdaf262 100644
--- a/clang/lib/Serialization/ASTWriterStmt.cpp
+++ b/clang/lib/Serialization/ASTWriterStmt.cpp
@@ -474,14 +474,12 @@ addConstraintSatisfaction(ASTRecordWriter &Record,
   if (!Satisfaction.IsSatisfied) {
     Record.push_back(Satisfaction.NumRecords);
     for (const auto &DetailRecord : Satisfaction) {
-      Record.writeStmtRef(DetailRecord.first);
-      auto *E = DetailRecord.second.dyn_cast<Expr *>();
-      Record.push_back(E == nullptr);
+      auto *E = DetailRecord.dyn_cast<Expr *>();
+      Record.push_back(/* IsDiagnostic */ E == nullptr);
       if (E)
         Record.AddStmt(E);
       else {
-        auto *Diag = DetailRecord.second.get<std::pair<SourceLocation,
-                                                       StringRef> *>();
+        auto *Diag = DetailRecord.get<std::pair<SourceLocation, StringRef> *>();
         Record.AddSourceLocation(Diag->first);
         Record.AddString(Diag->second);
       }

[mizvekov]

LGTM, I agree that we currently don't use it, so we might as well remove it.

But the overall situation is all wrong, we shouldn't be storing fully formed diagnostic strings in the AST, we should be storing them as PartialDiagnostics.

But making that right requires bigger changes.