[clang-doc] Centralize TypeInfo creation.

[brettw]

Summary

Several different places in the code had similar computations for the parameters that were eventually passed to the TypeInfo constructor.

This centralizes that code in one function, and allows passing TypeInfo to the various other *Info structures that need it.

Remove some "auto" types and replace with the real type for getting declarations. This was making some duplicate checking difficult to see.

[paulkirth]

Thanks for the code cleanup. The patch is LGTM, with some small nits.

clang-tools-extra/clang-doc/Serialize.cpp

240

nit: can we invert this condition and use an early return to reduce nesting?

[paulkirth]

Also, can you take a look at the premerge check failure? it looks like it had trouble applying your patch, so you may need to rebase.

[brettw]

Comment addressed

clang-tools-extra/clang-doc/Representation.h

struct Reference {
   // Indicates if the info's parent is the global namespace, or if the info is
   // the global namespace
   bool IsInGlobalNamespace = false;
 };
 
 // A base struct for TypeInfos
 struct TypeInfo {
   TypeInfo() = default;
+  TypeInfo(const Reference &R) : Type(R) {}
+
   TypeInfo(SymbolID Type, StringRef Field, InfoType IT)
       : Type(Type, Field, IT) {}
   TypeInfo(SymbolID Type, StringRef Field, InfoType IT, StringRef Path)
       : Type(Type, Field, IT, Path) {}
   TypeInfo(llvm::StringRef RefName) : Type(RefName) {}
   TypeInfo(llvm::StringRef RefName, StringRef Path) : Type(RefName, Path) {}
 
   bool operator==(const TypeInfo &Other) const { return Type == Other.Type; }
 
   Reference Type; // Referenced type in this info.
 };
 
 // Info for field types.
 struct FieldTypeInfo : public TypeInfo {
   FieldTypeInfo() = default;
+  FieldTypeInfo(const TypeInfo &TI, StringRef Name = StringRef(),
+                StringRef DefaultValue = StringRef())
+      : TypeInfo(TI), Name(Name), DefaultValue(DefaultValue) {}
   FieldTypeInfo(SymbolID Type, StringRef Field, InfoType IT, StringRef Path,
                 llvm::StringRef Name)
       : TypeInfo(Type, Field, IT, Path), Name(Name) {}
   FieldTypeInfo(llvm::StringRef RefName, llvm::StringRef Name)
       : TypeInfo(RefName), Name(Name) {}
   FieldTypeInfo(llvm::StringRef RefName, StringRef Path, llvm::StringRef Name)
       : TypeInfo(RefName, Path), Name(Name) {}
 
   bool operator==(const FieldTypeInfo &Other) const {
     return std::tie(Type, Name, DefaultValue) ==
            std::tie(Other.Type, Other.Name, Other.DefaultValue);
   }
 
   SmallString<16> Name; // Name associated with this info.
 
   // When used for function parameters, contains the string representing the
   // expression of the default value, if any.
   SmallString<16> DefaultValue;
 };
 
 // Info for member types.
 struct MemberTypeInfo : public FieldTypeInfo {
   MemberTypeInfo() = default;
+  MemberTypeInfo(const TypeInfo &TI, StringRef Name, AccessSpecifier Access)
+      : FieldTypeInfo(TI, Name), Access(Access) {}
   MemberTypeInfo(SymbolID Type, StringRef Field, InfoType IT, StringRef Path,
                  llvm::StringRef Name, AccessSpecifier Access)
       : FieldTypeInfo(Type, Field, IT, Path, Name), Access(Access) {}
   MemberTypeInfo(llvm::StringRef RefName, llvm::StringRef Name,
                  AccessSpecifier Access)
       : FieldTypeInfo(RefName, Name), Access(Access) {}
   MemberTypeInfo(llvm::StringRef RefName, StringRef Path, llvm::StringRef Name,
                  AccessSpecifier Access)

clang-tools-extra/clang-doc/Serialize.cpp

 
 static SymbolID getUSRForDecl(const Decl *D) {
   llvm::SmallString<128> USR;
   if (index::generateUSRForDecl(D, USR))
     return SymbolID();
   return hashUSR(USR);
 }
 
-static RecordDecl *getDeclForType(const QualType &T) {
+static TagDecl *getTagDeclForType(const QualType &T) {
+  if (const TagDecl *D = T->getAsTagDecl())
+    return D->getDefinition();
+  return nullptr;
+}
+
+static RecordDecl *getRecordDeclForType(const QualType &T) {
   if (const RecordDecl *D = T->getAsRecordDecl())
     return D->getDefinition();
   return nullptr;
 }
 
+TypeInfo getTypeInfoForType(const QualType &T) {
+  const TagDecl *TD = getTagDeclForType(T);
+  if (!TD)
+    return TypeInfo(Reference(T.getAsString()));
+
+  InfoType IT;
+  if (dyn_cast<EnumDecl>(TD)) {
+    IT = InfoType::IT_enum;
+  } else if (dyn_cast<RecordDecl>(TD)) {
+    IT = InfoType::IT_record;
+  } else {
+    IT = InfoType::IT_default;
+  }
+  return TypeInfo(Reference(getUSRForDecl(TD), TD->getNameAsString(), IT,
+                            getInfoRelativePath(TD)));
+}
+
 static bool isPublic(const clang::AccessSpecifier AS,
                      const clang::Linkage Link) {
   if (AS == clang::AccessSpecifier::AS_private)
     return false;
   else if ((Link == clang::Linkage::ModuleLinkage) ||
            (Link == clang::Linkage::ExternalLinkage))
     return true;
   return false; // otherwise, linkage is some form of internal linkage

 
 // The Access parameter is only provided when parsing the field of an inherited
 // record, the access specification of the field depends on the inheritance mode
 static void parseFields(RecordInfo &I, const RecordDecl *D, bool PublicOnly,
                         AccessSpecifier Access = AccessSpecifier::AS_public) {
   for (const FieldDecl *F : D->fields()) {
     if (!shouldSerializeInfo(PublicOnly, /*IsInAnonymousNamespace=*/false, F))
       continue;
-    if (const auto *T = getDeclForType(F->getTypeSourceInfo()->getType())) {
-      // Use getAccessUnsafe so that we just get the default AS_none if it's not
-      // valid, as opposed to an assert.
-      if (const auto *N = dyn_cast<EnumDecl>(T)) {
-        I.Members.emplace_back(
-            getUSRForDecl(T), N->getNameAsString(), InfoType::IT_enum,
-            getInfoRelativePath(N), F->getNameAsString(),
-            getFinalAccessSpecifier(Access, N->getAccessUnsafe()));
-        continue;
-      } else if (const auto *N = dyn_cast<RecordDecl>(T)) {
-        I.Members.emplace_back(
-            getUSRForDecl(T), N->getNameAsString(), InfoType::IT_record,
-            getInfoRelativePath(N), F->getNameAsString(),
-            getFinalAccessSpecifier(Access, N->getAccessUnsafe()));
-        continue;
-      }
-    }
-
-    auto& member = I.Members.emplace_back(
-        F->getTypeSourceInfo()->getType().getAsString(), F->getNameAsString(),
+
+    MemberTypeInfo &NewMember = I.Members.emplace_back(
+        getTypeInfoForType(F->getTypeSourceInfo()->getType()),
+        F->getNameAsString(),
         getFinalAccessSpecifier(Access, F->getAccessUnsafe()));
-    populateMemberTypeInfo(member, F);
+    populateMemberTypeInfo(NewMember, F);
   }
 }
 
 static void parseEnumerators(EnumInfo &I, const EnumDecl *D) {
   for (const EnumConstantDecl *E : D->enumerators()) {
     std::string ValueExpr;
     if (const Expr *InitExpr = E->getInitExpr())
       ValueExpr = getSourceCode(D, InitExpr->getSourceRange());
 
     SmallString<16> ValueStr;
     E->getInitVal().toString(ValueStr);
     I.Members.emplace_back(E->getNameAsString(), ValueStr, ValueExpr);
   }
 }
 
 static void parseParameters(FunctionInfo &I, const FunctionDecl *D) {
   for (const ParmVarDecl *P : D->parameters()) {
-    FieldTypeInfo *FieldInfo = nullptr;
-    if (const auto *T = getDeclForType(P->getOriginalType())) {
-      if (const auto *N = dyn_cast<EnumDecl>(T)) {
-        FieldInfo = &I.Params.emplace_back(
-            getUSRForDecl(N), N->getNameAsString(), InfoType::IT_enum,
-            getInfoRelativePath(N), P->getNameAsString());
-      } else if (const auto *N = dyn_cast<RecordDecl>(T)) {
-        FieldInfo = &I.Params.emplace_back(
-            getUSRForDecl(N), N->getNameAsString(), InfoType::IT_record,
-            getInfoRelativePath(N), P->getNameAsString());
-      }
-      // Otherwise fall through to the default case below.
-    }
-
-    if (!FieldInfo) {
-      FieldInfo = &I.Params.emplace_back(P->getOriginalType().getAsString(),
-                                         P->getNameAsString());
-    }
+    FieldTypeInfo &FieldInfo = I.Params.emplace_back(
+        getTypeInfoForType(P->getOriginalType()), P->getNameAsString());
 
     if (const Expr *DefaultArg = P->getDefaultArg()) {
-      FieldInfo->DefaultValue = getSourceCode(D, DefaultArg->getSourceRange());
+      FieldInfo.DefaultValue = getSourceCode(D, DefaultArg->getSourceRange());
     }
   }
 }
 
 // TODO: Remove the serialization of Parents and VirtualParents, this
 // information is also extracted in the other definition of parseBases.
 static void parseBases(RecordInfo &I, const CXXRecordDecl *D) {
   // Don't parse bases if this isn't a definition.
   if (!D->isThisDeclarationADefinition())
     return;
   for (const CXXBaseSpecifier &B : D->bases()) {
     if (B.isVirtual())
       continue;
     if (const auto *Ty = B.getType()->getAs<TemplateSpecializationType>()) {
       const TemplateDecl *D = Ty->getTemplateName().getAsTemplateDecl();
       I.Parents.emplace_back(getUSRForDecl(D), B.getType().getAsString(),
                              InfoType::IT_record);
-    } else if (const RecordDecl *P = getDeclForType(B.getType()))
+    } else if (const RecordDecl *P = getRecordDeclForType(B.getType()))
       I.Parents.emplace_back(getUSRForDecl(P), P->getNameAsString(),
                              InfoType::IT_record, getInfoRelativePath(P));
     else
       I.Parents.emplace_back(B.getType().getAsString());
   }
   for (const CXXBaseSpecifier &B : D->vbases()) {
-    if (const auto *P = getDeclForType(B.getType()))
+    if (const RecordDecl *P = getRecordDeclForType(B.getType()))
       I.VirtualParents.emplace_back(getUSRForDecl(P), P->getNameAsString(),
                                     InfoType::IT_record,
                                     getInfoRelativePath(P));
     else
       I.VirtualParents.emplace_back(B.getType().getAsString());
   }
 }
 

 }
 
 static void populateFunctionInfo(FunctionInfo &I, const FunctionDecl *D,
                                  const FullComment *FC, int LineNumber,
                                  StringRef Filename, bool IsFileInRootDir,
                                  bool &IsInAnonymousNamespace) {
   populateSymbolInfo(I, D, FC, LineNumber, Filename, IsFileInRootDir,
                      IsInAnonymousNamespace);
-  if (const auto *T = getDeclForType(D->getReturnType())) {
-    if (isa<EnumDecl>(T))
-      I.ReturnType = TypeInfo(getUSRForDecl(T), T->getNameAsString(),
-                              InfoType::IT_enum, getInfoRelativePath(T));
-    else if (isa<RecordDecl>(T))
-      I.ReturnType = TypeInfo(getUSRForDecl(T), T->getNameAsString(),
-                              InfoType::IT_record, getInfoRelativePath(T));
-  } else {
-    I.ReturnType = TypeInfo(D->getReturnType().getAsString());
-  }
+  I.ReturnType = getTypeInfoForType(D->getReturnType());
   parseParameters(I, D);
 }
 
 static void populateMemberTypeInfo(MemberTypeInfo &I, const FieldDecl *D) {
   assert(D && "Expect non-null FieldDecl in populateMemberTypeInfo");
 
   ASTContext& Context = D->getASTContext();
   // TODO investigate whether we can use ASTContext::getCommentForDecl instead