From 4ce2cfb03409ee1f8819544ccdd301d5c6885a83 Mon Sep 17 00:00:00 2001
From: "Matt McCutchen (Correct Computation)" <matt@correctcomputation.com>
Date: Wed, 1 Sep 2021 15:13:19 -0400
Subject: [PATCH] Fix de-nesting of unnamed TagDecls and improve some comments.

---
 clang/lib/3C/DeclRewriter.cpp | 35 +++++++++++++++++++++++++----------
 clang/lib/3C/MultiDecls.cpp   | 13 +++++++++++--
 2 files changed, 36 insertions(+), 12 deletions(-)

diff --git a/clang/lib/3C/DeclRewriter.cpp b/clang/lib/3C/DeclRewriter.cpp
index af73dbe36f97..39a7f3e4b109 100644
--- a/clang/lib/3C/DeclRewriter.cpp
+++ b/clang/lib/3C/DeclRewriter.cpp
@@ -274,15 +274,30 @@ void DeclRewriter::denestTagDecls() {
     std::string DefinitionStr = R.getRewrittenText(CSR);
     // Delete the definition from the old location.
     rewriteSourceRange(R, CSR, "");
-    // We want to insert RD as a new child of its original semantic DeclContext,
-    // just before the existing child of that DeclContext of which RD was
-    // originally a descendant.
-    DeclContext *TopChild = TD;
-    while (TopChild->getLexicalParent() != TD->getDeclContext()) {
-      TopChild = TopChild->getLexicalParent();
+    // We want to find the nearest ancestor DeclContext of TD that is _not_ a
+    // TagDecl and make TD a child of that DeclContext (named `Parent` below),
+    // just before the child `TopTagDecl` of `Parent` of which TD was originally
+    // a descendant.
+    //
+    // As of this writing, it seems that if TD is named, we get
+    // `Parent == TD->getDeclContext()` due to the code at
+    // https://github.com/correctcomputation/checkedc-clang/blob/fd4d8af4383d40af10ee8bc92b7bf88061a11035/clang/lib/Sema/SemaDecl.cpp#L16980-L16981,
+    // But that code doesn't run if TD is unnamed (which makes some sense
+    // because name visibility isn't an issue for TagDecls that have no name),
+    // and we want to de-nest TagDecls with names we assigned just like ones
+    // that were originally named, so we can't just use `TD->getDeclContext()`.
+    // In any event, maybe we wouldn't want to rely on this kind of internal
+    // Clang behavior.
+    DeclContext *TopTagDecl = TD;
+    for (;;) {
+      DeclContext *Parent = TopTagDecl->getLexicalParent();
+      if (!isa<TagDecl>(Parent))
+        break;
+      TopTagDecl = Parent;
     }
-    // TODO: Use a wrapper like rewriteSourceRange.
-    R.InsertText(cast<Decl>(TopChild)->getBeginLoc(), DefinitionStr);
+    // TODO: Use a wrapper like rewriteSourceRange that tries harder with
+    // macros, reports failure, etc.
+    R.InsertText(cast<Decl>(TopTagDecl)->getBeginLoc(), DefinitionStr);
   }
 }
 
@@ -332,8 +347,8 @@ void DeclRewriter::rewriteMultiDecl(MultiDeclInfo &MDI, RSet &ToRewrite) {
         llvm_unreachable("Failed to find place to insert assigned TagDecl name.");
       }
     }
-    // Make a note if the RecordDecl needs to be de-nested later.
-    if (TD->getLexicalDeclContext() != TD->getDeclContext())
+    // Make a note if the TagDecl needs to be de-nested later.
+    if (isa<TagDecl>(TD->getLexicalDeclContext()))
       TagDeclsToDenest.push_back(TD);
     // `struct T { ... } foo;` -> `struct T { ... };\nfoo;`
     rewriteSourceRange(R, TD->getEndLoc(), "};\n");
diff --git a/clang/lib/3C/MultiDecls.cpp b/clang/lib/3C/MultiDecls.cpp
index 48aeceeec039..2a94a51bd43f 100644
--- a/clang/lib/3C/MultiDecls.cpp
+++ b/clang/lib/3C/MultiDecls.cpp
@@ -133,6 +133,7 @@ void MultiDeclsInfo::findMultiDecls(DeclContext *DC, ASTContext &Context) {
           // Otherwise, just generate a new tag name based on the member name.
           // Example: `struct { ... } foo;` ->
           // `struct foo_struct_1 { ... }; struct foo_struct_1 foo;`
+          // If `foo_struct_1` is already taken, use `foo_struct_2`, etc.
           std::string KindName = std::string(LastTagDef->getKindName());
           std::string NewName;
           for (int Num = 1; ; Num++) {
@@ -142,8 +143,16 @@ void MultiDeclsInfo::findMultiDecls(DeclContext *DC, ASTContext &Context) {
           }
           AssignedTagTypeStrs.insert(std::make_pair(TagDefPSL, KindName + " " + NewName));
           TagDefNeedsName = false;
-          // This name is now taken; other automatically generated names must not
-          // collide with it.
+          // Consider this name taken and ensure that other automatically
+          // generated names do not collide with it.
+          //
+          // If the multi-decl doesn't end up getting rewritten, this name
+          // ultimately may not be used, creating a gap in the numbering in 3C's
+          // output. But this cosmetic inconsistency is a small price to pay for
+          // the architectural convenience of being able to store the assigned
+          // names in the PointerVariableConstraints when they are constructed
+          // rather than trying to assign and store the names after we know
+          // which multi-decls will be rewritten.
           UsedTagNames.insert(NewName);
         }
       }