Fix TagDecl de-nesting when the outer TagDecl is preceded by qualifiers.

TODO: Add regression tests.

Author: mattmccutchen-cci

clang/include/clang/3C/MultiDecls.h

@@ -158,10 +158,21 @@ struct MultiDeclInfo {
   bool AlreadyRewritten = false;
 };
 
-// All multi-decls, keyed by the common beginning source location of their
-// members. Note that the beginning source location of TagDefToSplit may be
-// later if there is a keyword such as `static` or `typedef` in between.
-typedef std::map<SourceLocation, MultiDeclInfo> TUMultiDeclsInfo;
+struct TUMultiDeclsInfo {
+  // All multi-decls, keyed by the common beginning source location of their
+  // members. Note that the beginning source location of TagDefToSplit may be
+  // later if there is a keyword such as `static` or `typedef` in between.
+  std::map<SourceLocation, MultiDeclInfo> MultiDeclsByBeginLoc;
+
+  // Map from a tag definition to its containing multi-decl (if it is part of
+  // one). Note that the TagDefToSplit of the MultiDeclInfo is not guaranteed to
+  // equal the TagDecl: it may be null in the `typedef struct { ... } T` case.
+  //
+  // Note that the MultiDeclInfo pointers remain valid for as long as the
+  // MultiDeclInfo objects remain in MultiDeclsByBeginLoc: see
+  // https://en.cppreference.com/w/cpp/container#Iterator_invalidation.
+  std::map<TagDecl *, MultiDeclInfo *> ContainingMultiDeclOfTagDecl;
+};
 
 class ProgramMultiDeclsInfo {
 private:
@@ -200,6 +211,7 @@ class ProgramMultiDeclsInfo {
   llvm::Optional<std::string> getTypeStrOverride(const Type *Ty,
                                                  const ASTContext &C);
   MultiDeclInfo *findContainingMultiDecl(MultiDeclMemberDecl *MMD);
+  MultiDeclInfo *findContainingMultiDecl(TagDecl *TD);
   bool wasBaseTypeRenamed(Decl *D);
 };
 

clang/lib/3C/DeclRewriter.cpp

@@ -276,16 +276,35 @@ void DeclRewriter::denestTagDecls() {
     // 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;
-    }
+    TagDecl *TopTagDecl = TD;
+    while (TagDecl *ParentTagDecl =
+               dyn_cast<TagDecl>(TopTagDecl->getLexicalDeclContext()))
+      TopTagDecl = ParentTagDecl;
+    // If TopTagDecl is preceded by qualifiers, ideally we'd like to insert TD
+    // before those qualifiers. If TopTagDecl is actually part of a multi-decl
+    // with at least one member, then we can just use the begin location of that
+    // multi-decl as the insertion point.
+    //
+    // If there are no members (so the qualifiers have no effect and generate a
+    // compiler warning), then there isn't an easy way for us to find the source
+    // location before the qualifiers. In that case, we just insert TD at the
+    // begin location of TopTagDecl (after the qualifiers) and hope for the
+    // best. In the cases we're aware of so far (storage qualifiers, including
+    // `typedef`), this just means that the qualifiers end up applying to the
+    // first TagDecl de-nested from TopTagDecl instead of to TopTagDecl itself,
+    // and they generate the same compiler warning as before but on a different
+    // TagDecl. However, we haven't confirmed that there aren't any obscure
+    // cases that could lead to an error, such as if a qualifier is valid on one
+    // kind of TagDecl but not another.
+    SourceLocation InsertLoc;
+    if (MultiDeclInfo *MDI =
+            Info.TheMultiDeclsInfo.findContainingMultiDecl(TopTagDecl))
+      InsertLoc = MDI->Members[0]->getBeginLoc();
+    else
+      InsertLoc = TopTagDecl->getBeginLoc();
     // TODO: Use a wrapper like rewriteSourceRange that tries harder with
     // macros, reports failure, etc.
-    R.InsertText(cast<Decl>(TopTagDecl)->getBeginLoc(), DefinitionStr);
+    R.InsertText(InsertLoc, DefinitionStr);
   }
 }
 

clang/lib/3C/MultiDecls.cpp

@@ -111,7 +111,7 @@ void ProgramMultiDeclsInfo::findMultiDecls(DeclContext *DC, ASTContext &Context)
       if (CurrentMultiDecl == nullptr || MMD->getBeginLoc() != CurrentBeginLoc) {
         // We are starting a new multi-decl.
         CurrentBeginLoc = MMD->getBeginLoc();
-        CurrentMultiDecl = &TUInfo[CurrentBeginLoc];
+        CurrentMultiDecl = &TUInfo.MultiDeclsByBeginLoc[CurrentBeginLoc];
         assert(CurrentMultiDecl->Members.empty() &&
                "Multi-decl members are not consecutive in traversal order");
         TagDefNeedsName = false;
@@ -125,6 +125,7 @@ void ProgramMultiDeclsInfo::findMultiDecls(DeclContext *DC, ASTContext &Context)
             !Context.getSourceManager().isBeforeInTranslationUnit(
                 LastTagDef->getBeginLoc(), CurrentBeginLoc)) {
           CurrentMultiDecl->TagDefToSplit = LastTagDef;
+          TUInfo.ContainingMultiDeclOfTagDecl[LastTagDef] = CurrentMultiDecl;
 
           // Do we need to automatically name the TagDefToSplit?
           if (LastTagDef->getName().empty()) {
@@ -248,11 +249,11 @@ ProgramMultiDeclsInfo::getTypeStrOverride(const Type *Ty, const ASTContext &C) {
 
 MultiDeclInfo *
 ProgramMultiDeclsInfo::findContainingMultiDecl(MultiDeclMemberDecl *MMD) {
-  TUMultiDeclsInfo &TUInfo = TUInfos[&MMD->getASTContext()];
+  auto &MultiDeclsByLoc = TUInfos[&MMD->getASTContext()].MultiDeclsByBeginLoc;
   // Look for a MultiDeclInfo for the beginning location of MMD, then check that
   // the MultiDeclInfo actually contains MMD.
-  auto It = TUInfo.find(MMD->getBeginLoc());
-  if (It == TUInfo.end())
+  auto It = MultiDeclsByLoc.find(MMD->getBeginLoc());
+  if (It == MultiDeclsByLoc.end())
     return nullptr;
   MultiDeclInfo &MDI = It->second;
   // Hope we don't have multi-decls with so many members that this becomes a
@@ -262,6 +263,14 @@ ProgramMultiDeclsInfo::findContainingMultiDecl(MultiDeclMemberDecl *MMD) {
   return nullptr;
 }
 
+MultiDeclInfo *ProgramMultiDeclsInfo::findContainingMultiDecl(TagDecl *TD) {
+  auto &MDOfTD = TUInfos[&TD->getASTContext()].ContainingMultiDeclOfTagDecl;
+  auto It = MDOfTD.find(TD);
+  if (It == MDOfTD.end())
+    return nullptr;
+  return It->second;
+}
+
 bool ProgramMultiDeclsInfo::wasBaseTypeRenamed(Decl *D) {
   // We assume that the base type was renamed if and only if D belongs to a
   // multi-decl marked as having the base type renamed. It might be better to