[clang-doc] Fix link generation

[DiegoAstiazaran]

Summary

Before making a link to a reference it is required to check that the reference has a path (eg. primitives won't have paths).
This was done by checking if the path was empty; that worked because when generating paths the outdirectory was included, so if the path was assigned it had that outdirectory at least.
The path generation was changed, it's now only the composite of the namespaces without the outdirectory. So if the info is in the global namespace the path would be empty and the old check wouldn't work as expected.
A new attribute has been added to the Reference struct that indicates if the info's parent is the global namespace.
Paths generation now fails if the path is empty and if the info is not in the global namespace.

[juliehockett]

Under what circumstances, exactly, is the path not set where you would need to create a link despite that? Is it only in the global namespace case? If so, could you special-case that and ignore other empty paths?

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

137 ↗

(On Diff #210698)

Is this field actually set anywhere?

[DiegoAstiazaran]

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

137 ↗

(On Diff #210698)

Yes, in the constructors where a path is assigned.

[DiegoAstiazaran]

In D64958#1601228, @juliehockett wrote:

Under what circumstances, exactly, is the path not set where you would need to create a link despite that? Is it only in the global namespace case? If so, could you special-case that and ignore other empty paths?

It's not specifically in the GlobalNamespace info file. It happens in that one but also in any info whose parent namespace is the global namespace.

[DiegoAstiazaran]

Change attribute used to check special case of global namespace; IsPathValid changed to IsInGlobalNamespace. This attribute is true when its first parent is the global namespace or if the info is the global namespace,

[jakehehrlich]

The code looks fine to me but I don't understand the global details of this. Hopefully Julie can still review things from there but if not we don't get timely reviews then we can still land this.

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

140–141 ↗

(On Diff #211979)

Here and below perhaps favor putting the comments above the line rather than to the side to make the formatter produce nicer looking code.

clang-tools-extra/unittests/clang-doc/YAMLGeneratorTest.cpp

83–84 ↗

(On Diff #211979)

Maybe put the comment above this line so that the formatter doesn't get all wonky.

[DiegoAstiazaran]

Fix comments

[juliehockett]

LGTM with a comment correction

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

117 ↗

(On Diff #213128)

s/globalnamespace/global namespace

123 ↗

(On Diff #213128)

s/globalnamespace/global namespace

[DiegoAstiazaran]

Fix comments.

clang-tools-extra/trunk/clang-doc/BitcodeReader.cpp

llvm::Error parseRecord(Record R, unsigned ID, llvm::StringRef Blob,
   case REFERENCE_USR:
     return decodeRecord(R, I->USR, Blob);
   case REFERENCE_NAME:
     return decodeRecord(R, I->Name, Blob);
   case REFERENCE_TYPE:
     return decodeRecord(R, I->RefType, Blob);
   case REFERENCE_PATH:
     return decodeRecord(R, I->Path, Blob);
+  case REFERENCE_IS_IN_GLOBAL_NAMESPACE:
+    return decodeRecord(R, I->IsInGlobalNamespace, Blob);
   case REFERENCE_FIELD:
     return decodeRecord(R, F, Blob);
   default:
     return llvm::make_error<llvm::StringError>("Invalid field for Reference.\n",
                                                llvm::inconvertibleErrorCode());
   }
 }
 

clang-tools-extra/trunk/clang-doc/BitcodeWriter.h

enum RecordId {
   RECORD_DEFLOCATION,
   RECORD_LOCATION,
   RECORD_TAG_TYPE,
   RECORD_IS_TYPE_DEF,
   REFERENCE_USR,
   REFERENCE_NAME,
   REFERENCE_TYPE,
   REFERENCE_PATH,
+  REFERENCE_IS_IN_GLOBAL_NAMESPACE,
   REFERENCE_FIELD,
   RI_LAST,
   RI_FIRST = VERSION
 };
 
 static constexpr unsigned BlockIdCount = BI_LAST - BI_FIRST;
 static constexpr unsigned RecordIdCount = RI_LAST - RI_FIRST;
 

clang-tools-extra/trunk/clang-doc/BitcodeWriter.cpp

RecordIdNameMap = []() {
           {FUNCTION_DEFLOCATION, {"DefLocation", &LocationAbbrev}},
           {FUNCTION_LOCATION, {"Location", &LocationAbbrev}},
           {FUNCTION_ACCESS, {"Access", &IntAbbrev}},
           {FUNCTION_IS_METHOD, {"IsMethod", &BoolAbbrev}},
           {REFERENCE_USR, {"USR", &SymbolIDAbbrev}},
           {REFERENCE_NAME, {"Name", &StringAbbrev}},
           {REFERENCE_TYPE, {"RefType", &IntAbbrev}},
           {REFERENCE_PATH, {"Path", &StringAbbrev}},
+          {REFERENCE_IS_IN_GLOBAL_NAMESPACE,
+           {"IsInGlobalNamespace", &BoolAbbrev}},
           {REFERENCE_FIELD, {"Field", &IntAbbrev}}};
       assert(Inits.size() == RecordIdCount);
       for (const auto &Init : Inits) {
         RecordIdNameMap[Init.first] = Init.second;
         assert((Init.second.Name.size() + 1) <= BitCodeConstants::RecordSize);
       }
       assert(RecordIdNameMap.size() == RecordIdCount);
       return RecordIdNameMap;
RecordsByBlock{
           RECORD_LOCATION, RECORD_TAG_TYPE, RECORD_IS_TYPE_DEF}},
         // Function Block
         {BI_FUNCTION_BLOCK_ID,
          {FUNCTION_USR, FUNCTION_NAME, FUNCTION_DEFLOCATION, FUNCTION_LOCATION,
           FUNCTION_ACCESS, FUNCTION_IS_METHOD}},
         // Reference Block
         {BI_REFERENCE_BLOCK_ID,
          {REFERENCE_USR, REFERENCE_NAME, REFERENCE_TYPE, REFERENCE_PATH,
-          REFERENCE_FIELD}}};
+          REFERENCE_IS_IN_GLOBAL_NAMESPACE, REFERENCE_FIELD}}};
 
 // AbbreviationMap
 
 constexpr unsigned char BitCodeConstants::Signature[];
 
 void ClangDocBitcodeWriter::AbbreviationMap::add(RecordId RID,
                                                  unsigned AbbrevID) {
   assert(RecordIdNameMap[RID] && "Unknown RecordId.");

 void ClangDocBitcodeWriter::emitBlock(const Reference &R, FieldId Field) {
   if (R.USR == EmptySID && R.Name.empty())
     return;
   StreamSubBlockGuard Block(Stream, BI_REFERENCE_BLOCK_ID);
   emitRecord(R.USR, REFERENCE_USR);
   emitRecord(R.Name, REFERENCE_NAME);
   emitRecord((unsigned)R.RefType, REFERENCE_TYPE);
   emitRecord(R.Path, REFERENCE_PATH);
+  emitRecord(R.IsInGlobalNamespace, REFERENCE_IS_IN_GLOBAL_NAMESPACE);
   emitRecord((unsigned)Field, REFERENCE_FIELD);
 }
 
 void ClangDocBitcodeWriter::emitBlock(const TypeInfo &T) {
   StreamSubBlockGuard Block(Stream, BI_TYPE_BLOCK_ID);
   emitBlock(T.Type, FieldId::F_type);
 }
 

clang-tools-extra/trunk/clang-doc/HTMLGenerator.cpp

 static std::unique_ptr<TagNode> genLink(const Twine &Text, const Twine &Link) {
   auto LinkNode = llvm::make_unique<TagNode>(HTMLTag::TAG_A, Text);
   LinkNode->Attributes.try_emplace("href", Link.str());
   return LinkNode;
 }
 
 static std::unique_ptr<HTMLNode> genTypeReference(const Reference &Type,
                                                   StringRef CurrentDirectory) {
-  if (Type.Path.empty())
+  if (Type.Path.empty() && !Type.IsInGlobalNamespace)
     return llvm::make_unique<TextNode>(Type.Name);
   llvm::SmallString<128> Path =
       computeRelativePath(Type.Path, CurrentDirectory);
   llvm::sys::path::append(Path, Type.Name + ".html");
   // Paths in HTML must be in posix-style
   llvm::sys::path::native(Path, llvm::sys::path::Style::posix);
   return genLink(Type.Name, Path);
 }

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

llvm::SmallVector<SmallString<16>, 4>
       Args; // List of arguments to commands (for InlineCommand).
   std::vector<std::unique_ptr<CommentInfo>>
       Children; // List of child comments for this CommentInfo.
 };
 
 struct Reference {
   Reference() = default;
   Reference(llvm::StringRef Name) : Name(Name) {}
-  Reference(llvm::StringRef Name, StringRef Path) : Name(Name), Path(Path) {}
+  // An empty path means the info is in the global namespace because the path is
+  // a composite of the parent namespaces.
+  Reference(llvm::StringRef Name, StringRef Path)
+      : Name(Name), Path(Path), IsInGlobalNamespace(Path.empty()) {}
   Reference(SymbolID USR, StringRef Name, InfoType IT)
       : USR(USR), Name(Name), RefType(IT) {}
+  // An empty path means the info is in the global namespace because the path is
+  // a composite of the parent namespaces.
   Reference(SymbolID USR, StringRef Name, InfoType IT, StringRef Path)
-      : USR(USR), Name(Name), RefType(IT), Path(Path) {}
+      : USR(USR), Name(Name), RefType(IT), Path(Path),
+        IsInGlobalNamespace(Path.empty()) {}
 
   bool operator==(const Reference &Other) const {
     return std::tie(USR, Name, RefType) ==
            std::tie(Other.USR, Other.Name, Other.RefType);
   }
 
   SymbolID USR = SymbolID(); // Unique identifer for referenced decl
   SmallString<16> Name;      // Name of type (possibly unresolved).
   InfoType RefType = InfoType::IT_default; // Indicates the type of this
                                            // Reference (namespace, record,
                                            // function, enum, default).
-  llvm::SmallString<128> Path; // Path of directory where the clang-doc
-                               // generated file will be saved
+  // Path of directory where the clang-doc generated file will be saved
+  // (possibly unresolved)
+  llvm::SmallString<128> Path;
+  // 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(SymbolID Type, StringRef Field, InfoType IT)
       : Type(Type, Field, IT) {}
   TypeInfo(SymbolID Type, StringRef Field, InfoType IT, StringRef Path)

clang-tools-extra/trunk/clang-doc/YAMLGenerator.cpp

 };
 
 template <> struct MappingTraits<Reference> {
   static void mapping(IO &IO, Reference &Ref) {
     IO.mapOptional("Type", Ref.RefType, InfoType::IT_default);
     IO.mapOptional("Name", Ref.Name, SmallString<16>());
     IO.mapOptional("USR", Ref.USR, SymbolID());
     IO.mapOptional("Path", Ref.Path, SmallString<128>());
+    IO.mapOptional("IsInGlobalNamespace", Ref.IsInGlobalNamespace, false);
   }
 };
 
 template <> struct MappingTraits<TypeInfo> {
   static void mapping(IO &IO, TypeInfo &I) { TypeInfoMapping(IO, I); }
 };
 
 template <> struct MappingTraits<FieldTypeInfo> {

clang-tools-extra/trunk/unittests/clang-doc/YAMLGeneratorTest.cpp

TEST(YAMLGeneratorTest, emitRecordYAML) {
   I.Namespace.emplace_back(EmptySID, "A", InfoType::IT_namespace);
 
   I.DefLoc = Location(10, llvm::SmallString<16>{"test.cpp"});
   I.Loc.emplace_back(12, llvm::SmallString<16>{"test.cpp"});
 
   I.Members.emplace_back("int", "path/to/int", "X",
                          AccessSpecifier::AS_private);
   I.TagType = TagTypeKind::TTK_Class;
-  I.Parents.emplace_back(EmptySID, "F", InfoType::IT_record, "path/to/F");
+  // F is in the global namespace
+  I.Parents.emplace_back(EmptySID, "F", InfoType::IT_record, "");
   I.VirtualParents.emplace_back(EmptySID, "G", InfoType::IT_record,
                                 "path/to/G");
 
   I.ChildRecords.emplace_back(EmptySID, "ChildStruct", InfoType::IT_record);
   I.ChildFunctions.emplace_back();
   I.ChildFunctions.back().Name = "OneFunction";
   I.ChildEnums.emplace_back();
   I.ChildEnums.back().Name = "OneEnum";
Members:
   - Type:
       Name:            'int'
       Path:            'path/to/int'
     Name:            'X'
     Access:          Private
 Parents:
   - Type:            Record
     Name:            'F'
-    Path:            'path/to/F'
+    IsInGlobalNamespace: true
 VirtualParents:
   - Type:            Record
     Name:            'G'
     Path:            'path/to/G'
 ChildRecords:
   - Type:            Record
     Name:            'ChildStruct'
 ChildFunctions: