juliehockett added a reviewer: ioeric. juliehockett added inline comments.
================ Comment at: clang-tools-extra/clang-doc/ClangDoc.h:27 +struct ClangDocContext { + tooling::ExecutionContext *ECtx; ---------------- Can we put this in `Representation.h`? Here, you have a potential circular include with `Mapper.h`. ================ Comment at: clang-tools-extra/clang-doc/Mapper.cpp:37-40 serialize::emitInfo(D, getComment(D, D->getASTContext()), getLine(D, D->getASTContext()), - getFile(D, D->getASTContext()))); + getFile(D, D->getASTContext()), + CDCtx.PublicOnly)); ---------------- Since the emitInfos are returning `""` if they're being passed over, the results container is still storing them -- move the emission outside of this call, and only report if it returns a non-empty string. ================ Comment at: clang-tools-extra/clang-doc/Mapper.h:23 #include "clang/Tooling/Execution.h" +#include "ClangDoc.h" ---------------- See above, I don't love the potential for circular includes (since `Mapper.h` is included in the implementation file for `ClangDoc` ================ Comment at: clang-tools-extra/clang-doc/Serialize.cpp:174 -static void parseFields(RecordInfo &I, const RecordDecl *D) { +static bool shouldNotBePublic(const clang::AccessSpecifier as, const clang::Linkage link){ + if(as == clang::AccessSpecifier::AS_private) ---------------- For readability, it's usually better to use positives (i.e. `isPublic`). Then, if we ever want to check for internal only, `isPublic()` is much more readable than `!shouldNotBePublic()`. ================ Comment at: clang-tools-extra/clang-doc/Serialize.cpp:311-313 + bool nonPublicNamespace = (D->isAnonymousNamespace()) || + shouldNotBePublic(D->getAccess(), D->getLinkageInternal()); + if(PublicOnly && nonPublicNamespace){ ---------------- You can inline this. ================ Comment at: clang-tools-extra/clang-doc/tool/ClangDocMain.cpp:69 + "public", + llvm::cl::desc("Document only public structures."), + llvm::cl::init(false), llvm::cl::cat(ClangDocCategory)); ---------------- s/structures/declarations ================ Comment at: clang-tools-extra/test/clang-doc/yaml-module.cpp:14 +// CHECK-A: --- +// CHECK-A-NEXT: USR: '4429AA8706EF483A44B1DCE2D956BF0FEF82A9B7' +// CHECK-A-NEXT: Name: 'moduleFunction' ---------------- I'm working on making these less brittle (so changing the USR schema doesn't require changing all these tests and just check the length with a regex, so let's just do that here (see D48341's tests for an example). ================ Comment at: clang-tools-extra/test/clang-doc/yaml-module.cpp:18 +// CHECK-A-NEXT: - LineNumber: 12 +// CHECK-A-NEXT: Filename: 'test' +// CHECK-A-NEXT: Params: ---------------- To be system-agnostic, make this `{{.*}}` to match regardless of from where the test is run. ================ Comment at: clang-tools-extra/test/clang-doc/yaml-public-module.cpp:11-15 + + + + + ---------------- Unnecessary whitespace https://reviews.llvm.org/D48395 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits