[clang-doc] Fix bitcode writer

[DiegoAstiazaran]

Summary

Bitcode writer was not emitting the corresponding record for the Access attribute of a FunctionInfo. This has been added.
AS_none was being used as the default value for any AccessSpecifier attribute (in FunctionInfo and MemberTypeInfo), this has been changed to AS_public because this is the enum value that evaluates to 0.
The bitcode writer doesn't write values that are 0 so if an attribute was set to AS_public, this value is not written and after reading the bitcode it would have the default value which is AS_none. This is why the default value is now AS_public.

[DiegoAstiazaran]

Default value of AccessSpecifier attributes is now AS_public.
Multiple tests modified.

[juliehockett]

One comment then LGTM

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

507 ↗

(On Diff #214923)

In a C++ method, it should have an access associated with it, and so AS_none isn't a valid value.

[DiegoAstiazaran]

Use getAccess() instead of getAccessUnsafe() for CXXMethodDecl.
Add comments in YAML generator to specify that AS_none is used as the default here because it's the AS that shouldn't be part of the output. Even though AS_public is the default in the struct, it should be displayed in the YAML output.

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

 void ClangDocBitcodeWriter::emitBlock(const FunctionInfo &I) {
   StreamSubBlockGuard Block(Stream, BI_FUNCTION_BLOCK_ID);
   emitRecord(I.USR, FUNCTION_USR);
   emitRecord(I.Name, FUNCTION_NAME);
   for (const auto &N : I.Namespace)
     emitBlock(N, FieldId::F_namespace);
   for (const auto &CI : I.Description)
     emitBlock(CI);
+  emitRecord(I.Access, FUNCTION_ACCESS);
   emitRecord(I.IsMethod, FUNCTION_IS_METHOD);
   if (I.DefLoc)
     emitRecord(I.DefLoc.getValue(), FUNCTION_DEFLOCATION);
   for (const auto &L : I.Loc)
     emitRecord(L, FUNCTION_LOCATION);
   emitBlock(I.Parent, FieldId::F_parent);
   emitBlock(I.ReturnType);
   for (const auto &N : I.Params)

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

MemberTypeInfo(llvm::StringRef RefName, StringRef Path, llvm::StringRef Name,
                  AccessSpecifier Access)
       : FieldTypeInfo(RefName, Path, Name), Access(Access) {}
 
   bool operator==(const MemberTypeInfo &Other) const {
     return std::tie(Type, Name, Access) ==
            std::tie(Other.Type, Other.Name, Other.Access);
   }
 
-  AccessSpecifier Access = AccessSpecifier::AS_none; // Access level associated
-                                                     // with this info (public,
-                                                     // protected, private,
-                                                     // none).
+  // Access level associated with this info (public, protected, private, none).
+  // AS_public is set as default because the bitcode writer requires the enum
+  // with value 0 to be used as the default.
+  // (AS_public = 0, AS_protected = 1, AS_private = 2, AS_none = 3)
+  AccessSpecifier Access = AccessSpecifier::AS_public;
 };
 
 struct Location {
   Location() = default;
   Location(int LineNumber, SmallString<16> Filename)
       : LineNumber(LineNumber), Filename(std::move(Filename)) {}
   Location(int LineNumber, SmallString<16> Filename, bool IsFileInRootDir)
       : LineNumber(LineNumber), Filename(std::move(Filename)),
struct FunctionInfo : public SymbolInfo {
 
   void merge(FunctionInfo &&I);
 
   bool IsMethod = false; // Indicates whether this function is a class method.
   Reference Parent;      // Reference to the parent class decl for this method.
   TypeInfo ReturnType;   // Info about the return type of this function.
   llvm::SmallVector<FieldTypeInfo, 4> Params; // List of parameters.
   // Access level for this method (public, private, protected, none).
-  AccessSpecifier Access = AccessSpecifier::AS_none;
+  // AS_public is set as default because the bitcode writer requires the enum
+  // with value 0 to be used as the default.
+  // (AS_public = 0, AS_protected = 1, AS_private = 2, AS_none = 3)
+  AccessSpecifier Access = AccessSpecifier::AS_public;
 };
 
 // TODO: Expand to allow for documenting templating, inheritance access,
 // friend classes
 // Info for types.
 struct RecordInfo : public SymbolInfo {
   RecordInfo() : SymbolInfo(InfoType::IT_record) {}
   RecordInfo(SymbolID USR) : SymbolInfo(InfoType::IT_record, USR) {}

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

 
 std::pair<std::unique_ptr<Info>, std::unique_ptr<Info>>
 emitInfo(const FunctionDecl *D, const FullComment *FC, int LineNumber,
          llvm::StringRef File, bool IsFileInRootDir, bool PublicOnly) {
   FunctionInfo Func;
   bool IsInAnonymousNamespace = false;
   populateFunctionInfo(Func, D, FC, LineNumber, File, IsFileInRootDir,
                        IsInAnonymousNamespace);
+  Func.Access = clang::AccessSpecifier::AS_none;
   if (PublicOnly && ((IsInAnonymousNamespace ||
                       !isPublic(D->getAccess(), D->getLinkageInternal()))))
     return {};
 
-  Func.Access = clang::AccessSpecifier::AS_none;
-
   // Wrap in enclosing scope
   auto ParentI = std::make_unique<NamespaceInfo>();
   if (!Func.Namespace.empty())
     ParentI->USR = Func.Namespace[0].USR;
   else
     ParentI->USR = SymbolID();
   if (Func.Namespace.empty())
     ParentI->Path = getInfoRelativePath(ParentI->Namespace);

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

static void mapping(IO &IO, FieldTypeInfo &I) {
     TypeInfoMapping(IO, I);
     IO.mapOptional("Name", I.Name, SmallString<16>());
   }
 };
 
 template <> struct MappingTraits<MemberTypeInfo> {
   static void mapping(IO &IO, MemberTypeInfo &I) {
     FieldTypeInfoMapping(IO, I);
+    // clang::AccessSpecifier::AS_none is used as the default here because it's
+    // the AS that shouldn't be part of the output. Even though AS_public is the
+    // default in the struct, it should be displayed in the YAML output.
     IO.mapOptional("Access", I.Access, clang::AccessSpecifier::AS_none);
   }
 };
 
 template <> struct MappingTraits<NamespaceInfo> {
   static void mapping(IO &IO, NamespaceInfo &I) {
     InfoMapping(IO, I);
     IO.mapOptional("ChildNamespaces", I.ChildNamespaces,

 
 template <> struct MappingTraits<FunctionInfo> {
   static void mapping(IO &IO, FunctionInfo &I) {
     SymbolInfoMapping(IO, I);
     IO.mapOptional("IsMethod", I.IsMethod, false);
     IO.mapOptional("Parent", I.Parent, Reference());
     IO.mapOptional("Params", I.Params);
     IO.mapOptional("ReturnType", I.ReturnType);
+    // clang::AccessSpecifier::AS_none is used as the default here because it's
+    // the AS that shouldn't be part of the output. Even though AS_public is the
+    // default in the struct, it should be displayed in the YAML output.
     IO.mapOptional("Access", I.Access, clang::AccessSpecifier::AS_none);
   }
 };
 
 template <> struct MappingTraits<CommentInfo> {
   static void mapping(IO &IO, CommentInfo &I) { CommentInfoMapping(IO, I); }
 };
 

clang-tools-extra/trunk/test/clang-doc/single-file-public.cpp

 // CHECK-NEXT:     IsMethod:        true
 // CHECK-NEXT:     Parent:
 // CHECK-NEXT:         Type:            Record
 // CHECK-NEXT:         Name:            'Record'
 // CHECK-NEXT:         USR:             '{{[0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z][0-9A-Z]}}'
 // CHECK-NEXT:     ReturnType:
 // CHECK-NEXT:       Type:
 // CHECK-NEXT:         Name:            'void'
+// CHECK-NEXT:     Access:      Public
 // CHECK-NEXT: ...

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

TEST(BitcodeTest, emitFunctionInfoBitcode) {
   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.ReturnType = TypeInfo(EmptySID, "void", InfoType::IT_default);
   I.Params.emplace_back("int", "P");
 
+  I.Access = AccessSpecifier::AS_none;
+
   std::string WriteResult = writeInfo(&I);
   EXPECT_TRUE(WriteResult.size() > 0);
   std::vector<std::unique_ptr<Info>> ReadResults = readInfo(WriteResult, 1);
 
   CheckFunctionInfo(&I, InfoAsFunction(ReadResults[0].get()));
 }
 
 TEST(BitcodeTest, emitMethodInfoBitcode) {
   FunctionInfo I;
   I.Name = "f";
   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.ReturnType = TypeInfo(EmptySID, "void", InfoType::IT_default);
   I.Params.emplace_back("int", "P");
   I.IsMethod = true;
   I.Parent = Reference(EmptySID, "Parent", InfoType::IT_record);
 
-  // TODO: fix access
-  // I.Access = AccessSpecifier::AS_private;
+  I.Access = AccessSpecifier::AS_public;
 
   std::string WriteResult = writeInfo(&I);
   EXPECT_TRUE(WriteResult.size() > 0);
   std::vector<std::unique_ptr<Info>> ReadResults = readInfo(WriteResult, 1);
 
   CheckFunctionInfo(&I, InfoAsFunction(ReadResults[0].get()));
 }
 

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

TEST(HTMLGeneratorTest, emitNamespaceHTML) {
   I.Name = "Namespace";
   I.Namespace.emplace_back(EmptySID, "A", InfoType::IT_namespace);
 
   I.ChildNamespaces.emplace_back(EmptySID, "ChildNamespace",
                                  InfoType::IT_namespace, "Namespace");
   I.ChildRecords.emplace_back(EmptySID, "ChildStruct", InfoType::IT_record,
                               "Namespace");
   I.ChildFunctions.emplace_back();
+  I.ChildFunctions.back().Access = AccessSpecifier::AS_none;
   I.ChildFunctions.back().Name = "OneFunction";
   I.ChildEnums.emplace_back();
   I.ChildEnums.back().Name = "OneEnum";
 
   auto G = getHTMLGenerator();
   assert(G);
   std::string Buffer;
   llvm::raw_string_ostream Actual(Buffer);
<div>
   <ul>
     <li>
       <a href="r/ChildStruct.html">ChildStruct</a>
     </li>
   </ul>
   <h2 id="Functions">Functions</h2>
   <div>
     <h3 id="0000000000000000000000000000000000000000">OneFunction</h3>
-    <p>OneFunction()</p>
+    <p>public OneFunction()</p>
   </div>
   <h2 id="Enums">Enums</h2>
   <div>
     <h3 id="0000000000000000000000000000000000000000">enum OneEnum</h3>
   </div>
 </div>
 )raw";
 
   EXPECT_EQ(Expected, Actual.str());
 }
 
 TEST(HTMLGeneratorTest, emitFunctionHTML) {
   FunctionInfo I;
   I.Name = "f";
   I.Namespace.emplace_back(EmptySID, "A", InfoType::IT_namespace);
 
   I.DefLoc = Location(10, llvm::SmallString<16>{"dir/test.cpp"}, false);
   I.Loc.emplace_back(12, llvm::SmallString<16>{"test.cpp"});
 
+  I.Access = AccessSpecifier::AS_none;
+
   SmallString<16> PathTo;
   llvm::sys::path::native("path/to", PathTo);
   I.ReturnType = TypeInfo(EmptySID, "float", InfoType::IT_default, PathTo);
   I.Params.emplace_back("int", PathTo, "P");
   I.IsMethod = true;
   I.Parent = Reference(EmptySID, "Parent", InfoType::IT_record);
 
   auto G = getHTMLGenerator();

 
 TEST(HTMLGeneratorTest, emitCommentHTML) {
   FunctionInfo I;
   I.Name = "f";
   I.DefLoc = Location(10, llvm::SmallString<16>{"test.cpp"});
   I.ReturnType = TypeInfo(EmptySID, "void", InfoType::IT_default);
   I.Params.emplace_back("int", "I");
   I.Params.emplace_back("int", "J");
+  I.Access = AccessSpecifier::AS_none;
 
   CommentInfo Top;
   Top.Kind = "FullComment";
 
   Top.Children.emplace_back(std::make_unique<CommentInfo>());
   CommentInfo *BlankLine = Top.Children.back().get();
   BlankLine->Kind = "ParagraphComment";
   BlankLine->Children.emplace_back(std::make_unique<CommentInfo>());

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

TEST(MDGeneratorTest, emitNamespaceMD) {
   I.Name = "Namespace";
   I.Namespace.emplace_back(EmptySID, "A", InfoType::IT_namespace);
 
   I.ChildNamespaces.emplace_back(EmptySID, "ChildNamespace",
                                  InfoType::IT_namespace);
   I.ChildRecords.emplace_back(EmptySID, "ChildStruct", InfoType::IT_record);
   I.ChildFunctions.emplace_back();
   I.ChildFunctions.back().Name = "OneFunction";
+  I.ChildFunctions.back().Access = AccessSpecifier::AS_none;
   I.ChildEnums.emplace_back();
   I.ChildEnums.back().Name = "OneEnum";
 
   auto G = getMDGenerator();
   assert(G);
   std::string Buffer;
   llvm::raw_string_ostream Actual(Buffer);
   auto Err = G->generateDocForInfo(&I, Actual, ClangDocContext());

 ChildStruct
 
 
 
 ## Functions
 
 ### OneFunction
 
-* OneFunction()*
+*public  OneFunction()*
 
 
 
 ## Enums
 
 | enum OneEnum |
 
 --

 TEST(MDGeneratorTest, emitFunctionMD) {
   FunctionInfo I;
   I.Name = "f";
   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.Access = AccessSpecifier::AS_none;
+
   I.ReturnType = TypeInfo(EmptySID, "void", InfoType::IT_default);
   I.Params.emplace_back("int", "P");
   I.IsMethod = true;
   I.Parent = Reference(EmptySID, "Parent", InfoType::IT_record);
 
   auto G = getMDGenerator();
   assert(G);
   std::string Buffer;

 
 TEST(MDGeneratorTest, emitCommentMD) {
   FunctionInfo I;
   I.Name = "f";
   I.DefLoc = Location(10, llvm::SmallString<16>{"test.cpp"});
   I.ReturnType = TypeInfo(EmptySID, "void", InfoType::IT_default);
   I.Params.emplace_back("int", "I");
   I.Params.emplace_back("int", "J");
+  I.Access = AccessSpecifier::AS_none;
 
   CommentInfo Top;
   Top.Kind = "FullComment";
 
   Top.Children.emplace_back(std::make_unique<CommentInfo>());
   CommentInfo *BlankLine = Top.Children.back().get();
   BlankLine->Kind = "ParagraphComment";
   BlankLine->Children.emplace_back(std::make_unique<CommentInfo>());

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

TEST(SerializeTest, emitNamespaceInfo) {
   NamespaceInfo *BWithFunction = InfoAsNamespace(Infos[4].get());
   NamespaceInfo ExpectedBWithFunction(EmptySID);
   FunctionInfo F;
   F.Name = "f";
   F.ReturnType = TypeInfo(EmptySID, "void", InfoType::IT_default);
   F.DefLoc = Location(0, llvm::SmallString<16>{"test.cpp"});
   F.Namespace.emplace_back(EmptySID, "B", InfoType::IT_namespace);
   F.Namespace.emplace_back(EmptySID, "A", InfoType::IT_namespace);
+  F.Access = AccessSpecifier::AS_none;
   ExpectedBWithFunction.ChildFunctions.emplace_back(std::move(F));
   CheckNamespaceInfo(&ExpectedBWithFunction, BWithFunction);
 }
 
 TEST(SerializeTest, emitAnonymousNamespaceInfo) {
   EmittedInfoList Infos;
   ExtractInfosFromCode("namespace { }", 2, /*Public=*/false, Infos);
 
ExtractInfosFromCode("int F() { class G {}; return 0; };", 1, /*Public=*/true,
                        Infos);
 
   NamespaceInfo *BWithFunction = InfoAsNamespace(Infos[0].get());
   NamespaceInfo ExpectedBWithFunction(EmptySID);
   FunctionInfo F;
   F.Name = "F";
   F.ReturnType = TypeInfo(EmptySID, "int", InfoType::IT_default);
   F.DefLoc = Location(0, llvm::SmallString<16>{"test.cpp"});
+  F.Access = AccessSpecifier::AS_none;
   ExpectedBWithFunction.ChildFunctions.emplace_back(std::move(F));
   CheckNamespaceInfo(&ExpectedBWithFunction, BWithFunction);
 }
 
 TEST(SerializeTest, emitInlinedFunctionInfo) {
   EmittedInfoList Infos;
   ExtractInfosFromCode("inline void F(int I) { };", 1, /*Public=*/true, Infos);
 
   NamespaceInfo *BWithFunction = InfoAsNamespace(Infos[0].get());
   NamespaceInfo ExpectedBWithFunction(EmptySID);
   FunctionInfo F;
   F.Name = "F";
   F.ReturnType = TypeInfo(EmptySID, "void", InfoType::IT_default);
   F.DefLoc = Location(0, llvm::SmallString<16>{"test.cpp"});
   F.Params.emplace_back("int", "I");
+  F.Access = AccessSpecifier::AS_none;
   ExpectedBWithFunction.ChildFunctions.emplace_back(std::move(F));
   CheckNamespaceInfo(&ExpectedBWithFunction, BWithFunction);
 }
 
 TEST(SerializeTest, ) {
   EmittedInfoList Infos;
   ExtractInfosFromCode(R"raw(class F {};
 class G {} ;
export double exportedModuleFunction(double y);)raw",
 
   NamespaceInfo *BWithFunction = InfoAsNamespace(Infos[0].get());
   NamespaceInfo ExpectedBWithFunction(EmptySID);
   FunctionInfo F;
   F.Name = "moduleFunction";
   F.ReturnType = TypeInfo(EmptySID, "int", InfoType::IT_default);
   F.Loc.emplace_back(0, llvm::SmallString<16>{"test.cpp"});
   F.Params.emplace_back("int", "x");
+  F.Access = AccessSpecifier::AS_none;
   ExpectedBWithFunction.ChildFunctions.emplace_back(std::move(F));
   CheckNamespaceInfo(&ExpectedBWithFunction, BWithFunction);
 
   NamespaceInfo *BWithExportedFunction = InfoAsNamespace(Infos[1].get());
   NamespaceInfo ExpectedBWithExportedFunction(EmptySID);
   FunctionInfo ExportedF;
   ExportedF.Name = "exportedModuleFunction";
   ExportedF.ReturnType = TypeInfo(EmptySID, "double", InfoType::IT_default);
   ExportedF.Loc.emplace_back(0, llvm::SmallString<16>{"test.cpp"});
   ExportedF.Params.emplace_back("double", "y");
+  ExportedF.Access = AccessSpecifier::AS_none;
   ExpectedBWithExportedFunction.ChildFunctions.emplace_back(
       std::move(ExportedF));
   CheckNamespaceInfo(&ExpectedBWithExportedFunction, BWithExportedFunction);
 }
 
 // Test serialization of child records in namespaces and other records
 TEST(SerializeTest, emitChildRecords) {
   EmittedInfoList Infos;

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

TEST(YAMLGeneratorTest, emitNamespaceYAML) {
   I.Namespace.emplace_back(EmptySID, "A", InfoType::IT_namespace);
 
   I.ChildNamespaces.emplace_back(EmptySID, "ChildNamespace",
                                  InfoType::IT_namespace, "path/to/A/Namespace");
   I.ChildRecords.emplace_back(EmptySID, "ChildStruct", InfoType::IT_record,
                               "path/to/A/Namespace");
   I.ChildFunctions.emplace_back();
   I.ChildFunctions.back().Name = "OneFunction";
+  I.ChildFunctions.back().Access = AccessSpecifier::AS_none;
   I.ChildEnums.emplace_back();
   I.ChildEnums.back().Name = "OneEnum";
 
   auto G = getYAMLGenerator();
   assert(G);
   std::string Buffer;
   llvm::raw_string_ostream Actual(Buffer);
   auto Err = G->generateDocForInfo(&I, Actual, ClangDocContext());

 ChildRecords:
   - Type:            Record
     Name:            'ChildStruct'
     Path:            'path/to/A/r'
 ChildFunctions:
   - USR:             '0000000000000000000000000000000000000000'
     Name:            'OneFunction'
     ReturnType:      {}
+    Access:          Public
 ChildEnums:
   - USR:             '0000000000000000000000000000000000000000'
     Name:            'OneEnum'
 ...
 )raw";
   EXPECT_EQ(Expected, Actual.str());
 }
 
 TEST(YAMLGeneratorTest, emitFunctionYAML) {
   FunctionInfo I;
   I.Name = "f";
   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.Access = AccessSpecifier::AS_none;
+
   I.ReturnType =
       TypeInfo(EmptySID, "void", InfoType::IT_default, "path/to/void");
   I.Params.emplace_back("int", "path/to/int", "P");
   I.IsMethod = true;
   I.Parent = Reference(EmptySID, "Parent", InfoType::IT_record);
 
   auto G = getYAMLGenerator();
   assert(G);

 
 TEST(YAMLGeneratorTest, emitCommentYAML) {
   FunctionInfo I;
   I.Name = "f";
   I.DefLoc = Location(10, llvm::SmallString<16>{"test.cpp"});
   I.ReturnType = TypeInfo(EmptySID, "void", InfoType::IT_default);
   I.Params.emplace_back("int", "I");
   I.Params.emplace_back("int", "J");
+  I.Access = AccessSpecifier::AS_none;
 
   CommentInfo Top;
   Top.Kind = "FullComment";
 
   Top.Children.emplace_back(std::make_unique<CommentInfo>());
   CommentInfo *BlankLine = Top.Children.back().get();
   BlankLine->Kind = "ParagraphComment";
   BlankLine->Children.emplace_back(std::make_unique<CommentInfo>());