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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
