[PATCH] D66238: [clang-doc] Serialize inherited attributes and methods

[Julie Hockett via Phabricator via cfe-commits]

juliehockett added inline comments.


================
Comment at: clang-tools-extra/clang-doc/BitcodeWriter.h:33
 // BitCodeConstants, though they can be added without breaking it.
 static const unsigned VersionNumber = 2;
 
----------------
I definitely haven't been particularly good about bumping this, but can you bump it? This is a decently large change.


================
Comment at: clang-tools-extra/clang-doc/Serialize.cpp:233
 
-static void parseFields(RecordInfo &I, const RecordDecl *D, bool PublicOnly) {
+static bool documentInfo(bool PublicOnly, bool IsInAnonymousNamespace,
+                         const NamedDecl *D) {
----------------
s/documentInfo/shouldSerializeInfo


================
Comment at: clang-tools-extra/clang-doc/Serialize.cpp:277
   for (const FieldDecl *F : D->fields()) {
-    if (PublicOnly && !isPublic(F->getAccessUnsafe(), F->getLinkageInternal()))
+    if (!documentInfo(PublicOnly, false, F))
       continue;
----------------
s/false/`/*IsInAnonymousNamespace=*/false` 


================
Comment at: clang-tools-extra/clang-doc/Serialize.cpp:436
+      if (const CXXRecordDecl *Base =
+              cast_or_null<CXXRecordDecl>(Ty->getDecl()->getDefinition())) {
+        bool IsVirtual = false;
----------------
Will `getDecl()` always return a non-null pointer? In the normal case I'd assume so, but the `cast_or_null` will only catch a null coming out of `getDefinition()` or the cast, not `getDecl`, so just want to check.


================
Comment at: clang-tools-extra/clang-doc/Serialize.cpp:437-439
+        bool IsVirtual = false;
+        if (B.isVirtual())
+          IsVirtual = true;
----------------
Is there a reason this isn't `bool IsVirtual = B.IsVirtual()`?


================
Comment at: clang-tools-extra/clang-doc/Serialize.cpp:440-446
+        SymbolID USR = getUSRForDecl(Base);
+        std::string BaseName = Base->getNameAsString();
+        if (const auto *Ty = B.getType()->getAs<TemplateSpecializationType>()) {
+          const TemplateDecl *D = Ty->getTemplateName().getAsTemplateDecl();
+          USR = getUSRForDecl(D);
+          BaseName = B.getType().getAsString();
+        }
----------------
Do this in an if/else -- `getNameAsString()` is a decently expensive operation, so only do it when you need it.


================
Comment at: clang-tools-extra/clang-doc/Serialize.cpp:447-450
+        I.Bases.emplace_back(
+            USR, BaseName, getInfoRelativePath(Base), IsVirtual,
+            getFinalAccessSpecifier(ParentAccess, B.getAccessSpecifier()),
+            IsParent);
----------------
It would be better to create the BaseRecordInfo and then move it into the vector, since the constructor will copy all those strings.


================
Comment at: clang-tools-extra/clang-doc/Serialize.cpp:454
+          if (const auto *MD = dyn_cast<CXXMethodDecl>(Decl)) {
+            // Don't inherit private methods
+            if (MD->getAccessUnsafe() == AccessSpecifier::AS_private ||
----------------
s/inherit/serialize


================
Comment at: clang-tools-extra/clang-doc/Serialize.cpp:460
+            FI.IsMethod = true;
+            // The seventh arg in populateParentNamespaces is a boolean
+            // passed by reference, its value is not relevant in here so
----------------
s/populateParentNamespaces/populateFunctionInfo ?


================
Comment at: clang-tools-extra/unittests/clang-doc/YAMLGeneratorTest.cpp:91
+  I.Bases.back().ChildFunctions.back().Name = "InheritedFunctionOne";
+  I.Bases.back().Members.emplace_back("int", "path/to/int", "X",
+                                      AccessSpecifier::AS_private);
----------------
For clarity, call this something else? Since it already has an `X` private member.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66238/new/

https://reviews.llvm.org/D66238