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