[PATCH] D153557: [clang][ExtractAPI] Add support for C++ classes

[Daniel Grumberg via Phabricator via cfe-commits]

dang added a comment.

Starting to come together nicely, but I think now would be a good time to write some tests. I am particularly interested in more complex situations like inheritance hierarchies.



================
Comment at: clang/include/clang/ExtractAPI/API.h:25
 #include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/Specifiers.h"
 #include "clang/ExtractAPI/AvailabilityInfo.h"
----------------
Inste


================
Comment at: clang/include/clang/ExtractAPI/API.h:637
+    return (Record->getKind() == RK_CXXClass ||
+            Record->getKind() == RK_Struct || Record->getKind() == RK_Union);
+  }
----------------
Not sure this is correct, the semantic relationship is that structs and unions can be classes. This expresses that all struct records and unions are CXXClassRecords. It would be best to keep these entirely separate as `classof` is used by the type casting functions, e.g. `llvm::dynamic_cast` and we certainly don't want arbitrary struct or union record to be cast-able to CXXClassRecord.


================
Comment at: clang/include/clang/ExtractAPI/API.h:771
 
+template <>
+struct has_function_signature<CXXMethodRecord> : public std::true_type {};
----------------
mega nit: I suggest grouping together the `has_function_signature` trait and the `has_access` trait separately, i.e. moving the empty line from 770 to between 772 and 773


================
Comment at: clang/include/clang/ExtractAPI/DeclarationFragments.h:187
 
+class AccessControl {
+public:
----------------
Should this be API.h?


================
Comment at: clang/include/clang/ExtractAPI/DeclarationFragments.h:198
+private:
+  StringRef Access;
+};
----------------
Instead of storing a reference to the string here, it might make sense to store the string itself, I am worried that for future usages in libclang the string references would go stale.


================
Comment at: clang/include/clang/ExtractAPI/ExtractAPIVisitor.h:318-319
+bool ExtractAPIVisitorBase<Derived>::WalkUpFromRecordDecl(
+    const RecordDecl *Decl) {
+  VisitRecordDecl(Decl);
+  return true;
----------------
Is it to short circuit to only call `VisitRecordDecl` if so I think that it should call `getDerivedExtractAPIVisitor().VisitRecordDecl`


================
Comment at: clang/include/clang/ExtractAPI/ExtractAPIVisitor.h:400
+  // FIXME: store AccessSpecifier given by inheritance
+  for (const auto BaseSpecifier : Decl->bases()) {
+    SymbolReference BaseClass;
----------------
Is there a way of only looking at public bases here? Non-public inheritance is not part of the API surface of the class.


================
Comment at: clang/lib/ExtractAPI/API.cpp:19
 #include "clang/AST/RawCommentList.h"
+#include "clang/ExtractAPI/DeclarationFragments.h"
 #include "clang/Index/USRGeneration.h"
----------------
AccessControl should move into API.h so we don't need to pull in DeclarationFragments.h


================
Comment at: clang/lib/ExtractAPI/API.cpp:44
 }
-
 } // namespace
----------------
This just pollutes the git history, would be best to remove this.


================
Comment at: clang/lib/ExtractAPI/Serialization/SymbolGraphSerializer.cpp:607
-  // correctly here.
-  Obj["accessLevel"] = "public";
   SmallVector<StringRef, 4> PathComponentsNames;
----------------
We still need all symbols to have an "accessLevel" specifier. Maybe the false_type overload of `serializeAccessMixin` could make "public" the default.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153557