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

[Daniel Grumberg via Phabricator via cfe-commits]

dang added a comment.

Looking pretty good, if you can address the last few bits of feedback I am happy to merge this.



================
Comment at: clang/include/clang/ExtractAPI/API.h:770
+template <>
+struct has_function_signature<CXXMethodRecord> : public std::true_type {};
+
----------------
Does `CXXInstanceMethodRecord` need one of these as well?


================
Comment at: clang/include/clang/ExtractAPI/DeclarationFragments.h:26
 #include "clang/AST/DeclObjC.h"
+#include "clang/Basic/Specifiers.h"
 #include "clang/Lex/MacroInfo.h"
----------------
Are these necessary?


================
Comment at: clang/include/clang/ExtractAPI/DeclarationFragments.h:190
+public:
+  AccessControl() = default;
+
----------------
For ergonomics would it make more sense to have a constructor that takes the string in directly and moves it to `Access`? I don't think there are any situation where you want to change the access string after the fact?


================
Comment at: clang/include/clang/ExtractAPI/DeclarationFragments.h:251
+  template <typename FunctionT>
+  static FunctionSignature getFunctionSignature(const FunctionT *Function) {
+    FunctionSignature Signature;
----------------
I assume the implementation of this moved because it was needed for C++ methods? Can you update the comment and maybe move the implementation to the bottom of the `.h` file?


================
Comment at: clang/lib/ExtractAPI/DeclarationFragments.cpp:614
+  } else if (isa<CXXDestructorDecl>(Method))
+    // TODO: add '~'
+    Name = cast<CXXRecordDecl>(Method->getDeclContext())->getName();
----------------
Please address this, it's pretty important imo.


================
Comment at: clang/test/ExtractAPI/methods.cpp:13
+
+// CHECK-NOT: error:
+// CHECK-NOT: warning:
----------------
I think these are redundant since you have `// expected-no-diagnostics` below and pass ``--verify`` to cc1.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153557