zixuw added a comment.
Good to see refactoring to make ExtractAPI easier to extend and use. Thanks
Daniel!
Some comments on the ExtractAPIVisitor part. I'll leave the libclang part to
the experts.
================
Comment at: clang/include/clang/ExtractAPI/ExtractAPIVisitor.h:32
+template <typename Derived>
+class ExtractAPIVisitorBase : public RecursiveASTVisitor<Derived> {
public:
----------------
Would it be better to call this `ExtractAPIVisitorImpl` because it provides the
visitor implementation, and the `ExtractAPIVisitor` is actually the base for
the second level CRTP for actual visitors?
================
Comment at: clang/include/clang/ExtractAPI/ExtractAPIVisitor.h:32
+template <typename Derived>
+class ExtractAPIVisitorBase : public RecursiveASTVisitor<Derived> {
public:
----------------
zixuw wrote:
> Would it be better to call this `ExtractAPIVisitorImpl` because it provides
> the visitor implementation, and the `ExtractAPIVisitor` is actually the base
> for the second level CRTP for actual visitors?
Should this be `: public RecursiveASTVisitor<ExtractAPIVisitorBase<Derived>>`
instead?
================
Comment at: clang/include/clang/ExtractAPI/ExtractAPIVisitor.h:34
public:
- ExtractAPIVisitor(ASTContext &Context,
- llvm::unique_function<bool(SourceLocation)>
LocationChecker,
- APISet &API)
- : Context(Context), API(API),
- LocationChecker(std::move(LocationChecker)) {}
+ ExtractAPIVisitorBase(ASTContext &Context, APISet &API)
+ : Context(Context), API(API) {}
----------------
Should the constructor be made `protected` for a CRTP base?
================
Comment at: clang/include/clang/ExtractAPI/ExtractAPIVisitor.h:57
+
+ const RawComment *fetchRawCommentForDecl(const Decl *Decl) const;
+
----------------
Why does comment-fetching need to be dispatched?
================
Comment at: clang/include/clang/ExtractAPI/ExtractAPIVisitor.h:102
+private:
+ Derived &getConcrete() { return *static_cast<Derived *>(this); }
+};
----------------
I see that the `RecursiveASTVisitor` already provides a `getDerived()` for the
top-level CRTP.
My head is still bending trying to get a clear view of the multi-level CRTP
here 🙃 , but I'm guessing that this is to avoid the conflict with that method.
In that case could we be more verbose to make clear the purpose of this one?
I'm thinking something like `getDerivedExtractAPIVisitor()`, to communicate
that this is for getting the derived/concrete ExtractAPIVisitor subclass.
================
Comment at: clang/lib/ExtractAPI/ExtractAPIConsumer.cpp:170
bool operator()(SourceLocation Loc) {
- // If the loc refers to a macro expansion we need to first get the file
+ // If the loc refersSourceLocationxpansion we need to first get the file
// location of the expansion.
----------------
bnbarham wrote:
> Accidental?
Missed change?
================
Comment at: clang/lib/ExtractAPI/ExtractAPIConsumer.cpp:228-234
+ // Check that we have the definition for redeclarable types.
+ if (auto *TD = llvm::dyn_cast<TagDecl>(D))
+ ShouldBeIncluded = TD->isThisDeclarationADefinition();
+ else if (auto *Interface = llvm::dyn_cast<ObjCInterfaceDecl>(D))
+ ShouldBeIncluded = Interface->isThisDeclarationADefinition();
+ else if (auto *Protocol = llvm::dyn_cast<ObjCProtocolDecl>(D))
+ ShouldBeIncluded = Protocol->isThisDeclarationADefinition();
----------------
Couldn't find the original logic in this patch. Could you remind me what are
these for? Also good to have more comments here to explain things.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D146656/new/
https://reviews.llvm.org/D146656
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
