[PATCH] D48395: Added PublicOnly flag

[Julie Hockett via Phabricator via cfe-commits]

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