Added PublicOnly flag

[anniecherk]

Summary

Added a flag which, when enabled, documents only those methods and fields which have a Public attribute.

[juliehockett]

Can you re-upload the patch with context? (i.e. use -U999999 or similar)

[juliehockett]

So on seeing this, all of the plumbing needed to get that particular value where you need it is a bit ugly. I'm inclined to suggest wrapping it in a ClangDocContext, containing it and the ExecutionContext in the ClangDoc.h file:

struct ClangDocContext {
    ExecutionContext &ECtx;
    bool PublicOnly;
}

and then basically just replace everywhere the ECtx is plumbed through with the new context object.

clang-tools-extra/clang-doc/Serialize.cpp

307

You also want to check access here (records can be private to a class).

334

Move this to the top of the function and do the check before you set the value on I -- if we're not going to return anything, you should exit quickly before you do all of the allocation.

340

Also check here

clang-tools-extra/clang-doc/tool/ClangDocMain.cpp

68

Can we make the flag just --public?

clang-tools-extra/test/clang-doc/yaml-record-public-only.cpp

10

Add a test case for a function.

[anniecherk]

• addressed Julie's comments
• updated logic to consider both access specifier & linkage
• added tests for each type of linkage

[anniecherk]

Updated the tests to more accurately check that the files that we expect to not be generated by clang-doc with the public flag are in fact not being created.

The tests now run clang-doc over the test files with and without the public flag, outputting yaml files into two different directories.
File-check then diffs those directories and verifies that the files we expect to be absent in the directory created from running clang-doc with the public flag are in fact absent.

[juliehockett]

clang-tools-extra/clang-doc/ClangDoc.h

27

Can we put this in Representation.h? Here, you have a potential circular include with Mapper.h.

clang-tools-extra/clang-doc/Mapper.cpp

37–40

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.

clang-tools-extra/clang-doc/Mapper.h

23

See above, I don't love the potential for circular includes (since Mapper.h is included in the implementation file for ClangDoc

clang-tools-extra/clang-doc/Serialize.cpp

174

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().

311–313

You can inline this.

clang-tools-extra/clang-doc/tool/ClangDocMain.cpp

69

s/structures/declarations

clang-tools-extra/test/clang-doc/yaml-module.cpp

14 ↗

(On Diff #153790)

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).

18 ↗

(On Diff #153790)

To be system-agnostic, make this {{.*}} to match regardless of from where the test is run.

clang-tools-extra/test/clang-doc/yaml-public-module.cpp

11–15 ↗

(On Diff #153790)

Unnecessary whitespace

[anniecherk]

Addressed Julie's comments

[juliehockett]

Please run clang-format and clang-tidy on the patch.

clang-tools-extra/clang-doc/ClangDoc.cpp

25

Sort includes (this should be at the top)

clang-tools-extra/clang-doc/Mapper.cpp

13

Sort includes alphabetically, but you shouldn't need this here?

clang-tools-extra/clang-doc/Mapper.h

23

Sort includes (this should be at the top)

clang-tools-extra/clang-doc/Serialize.cpp

174

clang-tidy will tell you to capitalize these in llvm world (as -> AS, link -> Link)

322–324

Since this is the same for Record/Function/Enum, can we move this to populateSymbolInfo()?

Also, elide braces on single-line ifs.

[anniecherk]

clang-tools-extra/clang-doc/Serialize.cpp

322–324

I don't see a good way to put it into populateSymbolInfo because if the condition passes then the emitInfo method needs to bail out and I don't see a clean way to do that if the check is in populateSymbolInfo. A clunky way to do this would be to either have populateSymbolInfo set a flag that emitInfo checks or emitInfo can assume that populateSymbolInfo bailed if all the info is unpopulated, but that seems like a worse way to do it. Am I missing something?

I can refactor the condition into a function if that would be better for understandability / maintainability.

[juliehockett]

clang-tools-extra/clang-doc/Serialize.cpp

322–324

Hmm I suppose this is fine then.

Still elide braces on single-line ifs.

[anniecherk]

Updated according to Julie's suggestions, mostly adjusted formatting.

[juliehockett]

Remember to mark comments as done when they are. Otherwise, LGTM unless @ioeric has any concerns.

[ioeric]

In D48395#1158869, @juliehockett wrote:

Remember to mark comments as done when they are. Otherwise, LGTM unless @ioeric has any concerns.

No concern if this looks good to Julie.

clang-tools-extra/test/clang-doc/yaml-module.cpp

14 ↗

(On Diff #154470)

This could be [0-9A-Z]{N} where N = length(USR), right?

[juliehockett]

clang-tools-extra/test/clang-doc/yaml-module.cpp

14 ↗

(On Diff #154470)

The {n} syntax doesn't work with FileCheck.

clang-tools-extra/clang-doc/ClangDoc.h

 namespace doc {
 
 std::unique_ptr<tooling::FrontendActionFactory>
-newMapperActionFactory(tooling::ExecutionContext *ECtx);
+newMapperActionFactory(tooling::ExecutionContext *ECtx, bool PublicOnly);
 
 } // namespace doc
 } // namespace clang

clang-tools-extra/clang-doc/ClangDoc.cpp

 
 class MapperActionFactory : public tooling::FrontendActionFactory {
 public:
-  MapperActionFactory(tooling::ExecutionContext *ECtx) : ECtx(ECtx) {}
+  MapperActionFactory(tooling::ExecutionContext *ECtx, bool PublicOnly) :
+      ECtx(ECtx), PublicOnly(PublicOnly) {}
   clang::FrontendAction *create() override;
 
 private:
   tooling::ExecutionContext *ECtx;
+  bool PublicOnly;
 };
 
 clang::FrontendAction *MapperActionFactory::create() {
   class ClangDocAction : public clang::ASTFrontendAction {
   public:
-    ClangDocAction(ExecutionContext *ECtx) : ECtx(ECtx) {}
+    ClangDocAction(ExecutionContext *ECtx, bool PublicOnly) : ECtx(ECtx), PublicOnly(PublicOnly) {}
 
     std::unique_ptr<clang::ASTConsumer>
     CreateASTConsumer(clang::CompilerInstance &Compiler,
                       llvm::StringRef InFile) override {
-      return llvm::make_unique<MapASTVisitor>(&Compiler.getASTContext(), ECtx);
+      return llvm::make_unique<MapASTVisitor>(&Compiler.getASTContext(), ECtx, PublicOnly);
     }
 
   private:
     ExecutionContext *ECtx;
+    bool PublicOnly;
   };
-  return new ClangDocAction(ECtx);
+  return new ClangDocAction(ECtx, PublicOnly);
 }
 
 std::unique_ptr<tooling::FrontendActionFactory>
-newMapperActionFactory(tooling::ExecutionContext *ECtx) {
-  return llvm::make_unique<MapperActionFactory>(ECtx);
+newMapperActionFactory(tooling::ExecutionContext *ECtx, bool PublicOnly) {
+  return llvm::make_unique<MapperActionFactory>(ECtx, PublicOnly);
 }
 
 } // namespace doc

clang-tools-extra/clang-doc/Mapper.h

 class MapASTVisitor : public clang::RecursiveASTVisitor<MapASTVisitor>,
                       public ASTConsumer {
 public:
-  explicit MapASTVisitor(ASTContext *Ctx, ExecutionContext *ECtx)
-      : ECtx(ECtx) {}
+  explicit MapASTVisitor(ASTContext *Ctx, ExecutionContext *ECtx, bool PublicOnly)
+      : ECtx(ECtx), PublicOnly(PublicOnly) {}
 
   void HandleTranslationUnit(ASTContext &Context) override;
   bool VisitNamespaceDecl(const NamespaceDecl *D);

                                     const ASTContext &Context) const;
 
   ExecutionContext *ECtx;
+  bool PublicOnly;
 };
 
 } // namespace doc

clang-tools-extra/clang-doc/Mapper.cpp

   ECtx->reportResult(llvm::toHex(llvm::toStringRef(serialize::hashUSR(USR))),
                      serialize::emitInfo(D, getComment(D, D->getASTContext()),
                                          getLine(D, D->getASTContext()),
-                                         getFile(D, D->getASTContext())));
+                                         getFile(D, D->getASTContext()),
+                                         PublicOnly));
   return true;
 }
 

clang-tools-extra/clang-doc/Serialize.h

 namespace serialize {
 
 std::string emitInfo(const NamespaceDecl *D, const FullComment *FC,
-                     int LineNumber, StringRef File);
+                     int LineNumber, StringRef File, bool PublicOnly);
 std::string emitInfo(const RecordDecl *D, const FullComment *FC, int LineNumber,
-                     StringRef File);
+                     StringRef File, bool PublicOnly);
 std::string emitInfo(const EnumDecl *D, const FullComment *FC, int LineNumber,
-                     StringRef File);
+                     StringRef File, bool PublicOnly);
 std::string emitInfo(const FunctionDecl *D, const FullComment *FC,
-                     int LineNumber, StringRef File);
+                     int LineNumber, StringRef File, bool PublicOnly);
 std::string emitInfo(const CXXMethodDecl *D, const FullComment *FC,
-                     int LineNumber, StringRef File);
+                     int LineNumber, StringRef File, bool PublicOnly);
 
 // Function to hash a given USR value for storage.
 // As USRs (Unified Symbol Resolution) could be large, especially for functions

clang-tools-extra/clang-doc/Serialize.cpp

   return Ty->getDecl()->getDefinition();
 }
 
-static void parseFields(RecordInfo &I, const RecordDecl *D) {
+static void parseFields(RecordInfo &I, const RecordDecl *D, bool PublicOnly) {
   for (const FieldDecl *F : D->fields()) {
+    if(PublicOnly && (F->getAccessUnsafe() != clang::AccessSpecifier::AS_public)){
+        continue;
+    }
     if (const auto *T = getDeclForType(F->getTypeSourceInfo()->getType())) {
       // Use getAccessUnsafe so that we just get the default AS_none if it's not
       // valid, as opposed to an assert.

 }
 
 std::string emitInfo(const NamespaceDecl *D, const FullComment *FC,
-                     int LineNumber, llvm::StringRef File) {
+                     int LineNumber, llvm::StringRef File, bool PublicOnly) {
   NamespaceInfo I;
   populateInfo(I, D, FC);
   return serialize(I);
 }
 
 std::string emitInfo(const RecordDecl *D, const FullComment *FC, int LineNumber,
-                     llvm::StringRef File) {
+                     llvm::StringRef File, bool PublicOnly) {
   RecordInfo I;
   populateSymbolInfo(I, D, FC, LineNumber, File);
   I.TagType = D->getTagKind();
-  parseFields(I, D);
+  parseFields(I, D, PublicOnly);
   if (const auto *C = dyn_cast<CXXRecordDecl>(D))
     parseBases(I, C);
   return serialize(I);
 }
 
 std::string emitInfo(const FunctionDecl *D, const FullComment *FC,
-                     int LineNumber, llvm::StringRef File) {
+                     int LineNumber, llvm::StringRef File, bool PublicOnly) {
   FunctionInfo I;
   populateFunctionInfo(I, D, FC, LineNumber, File);
   I.Access = clang::AccessSpecifier::AS_none;

 }
 
 std::string emitInfo(const CXXMethodDecl *D, const FullComment *FC,
-                     int LineNumber, llvm::StringRef File) {
+                     int LineNumber, llvm::StringRef File, bool PublicOnly) {
   FunctionInfo I;
   populateFunctionInfo(I, D, FC, LineNumber, File);
   I.IsMethod = true;
   I.Parent = Reference{getUSRForDecl(D->getParent()),
                        D->getParent()->getNameAsString(), InfoType::IT_record};
   I.Access = D->getAccess();
+  if(PublicOnly && (I.Access != clang::AccessSpecifier::AS_public)){
+    return "";
+  }
   return serialize(I);
 }
 
 std::string emitInfo(const EnumDecl *D, const FullComment *FC, int LineNumber,
-                     llvm::StringRef File) {
+                     llvm::StringRef File, bool PublicOnly) {
   EnumInfo I;
   populateSymbolInfo(I, D, FC, LineNumber, File);
   I.Scoped = D->isScoped();

clang-tools-extra/clang-doc/tool/ClangDocMain.cpp

     llvm::cl::desc("Dump intermediate results to bitcode file."),
     llvm::cl::init(false), llvm::cl::cat(ClangDocCategory));
 
+static llvm::cl::opt<bool> PublicOnly(
+    "public-only",
+    llvm::cl::desc("Only document public methods & public fields."),
+    llvm::cl::init(false), llvm::cl::cat(ClangDocCategory));
+
 enum OutputFormatTy {
   yaml,
 };

   // Mapping phase
   llvm::outs() << "Mapping decls...\n";
   auto Err = Exec->get()->execute(
-      doc::newMapperActionFactory(Exec->get()->getExecutionContext()),
+      doc::newMapperActionFactory(Exec->get()->getExecutionContext(), PublicOnly),
       ArgAdjuster);
   if (Err) {
     llvm::errs() << toString(std::move(Err)) << "\n";

clang-tools-extra/test/clang-doc/yaml-record-public-only.cpp

+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: echo "" > %t/compile_flags.txt
+// RUN: cp "%s" "%t/test.cpp"
+// RUN: clang-doc --public-only --doxygen -p %t %t/test.cpp -output=%t/docs
+// RUN: cat %t/docs/Struct.yaml | FileCheck %s --check-prefix=CHECK-A
+// RUN: cat %t/docs/Struct/publicMethod.yaml | FileCheck %s --check-prefix=CHECK-B
+// RUN: cat %t/docs/Class.yaml | FileCheck %s --check-prefix=CHECK-C
+// RUN: cat %t/docs/Class/publicMethod.yaml | FileCheck %s --check-prefix=CHECK-D
+
+struct Struct {
+  public:
+    void publicMethod();
+    int  publicField;
+ protected:
+    void protectedMethod();
+    int  protectedField;
+ private:
+    void privateMethod();
+    int  privateField;
+};
+
+
+// CHECK-A: ---
+// CHECK-A-NEXT: USR:             '0E0522198A2C7D141073719DA0815FCD6DC95610'
+// CHECK-A-NEXT: Name:            'Struct'
+// CHECK-A-NEXT: DefLocation:
+// CHECK-A-NEXT:   LineNumber:      11
+// CHECK-A-NEXT:   Filename:        'test'
+// CHECK-A-NEXT: Members:
+// CHECK-A-NEXT:   - Type:
+// CHECK-A-NEXT:       Name:            'int'
+// CHECK-A-NEXT:     Name:            'publicField'
+// CHECK-A-NEXT: ...
+
+
+// CHECK-B: ---
+// CHECK-B-NEXT: USR:             'DC71C1FF683E2DE57395B0029458ADC4BDDE46CA'
+// CHECK-B-NEXT: Name:            'publicMethod'
+// CHECK-B-NEXT: Namespace:
+// CHECK-B-NEXT:   - Type:            Record
+// CHECK-B-NEXT:     Name:            'Struct'
+// CHECK-B-NEXT:     USR:             '0E0522198A2C7D141073719DA0815FCD6DC95610'
+// CHECK-B-NEXT: Location:
+// CHECK-B-NEXT:   - LineNumber:      13
+// CHECK-B-NEXT:     Filename:        'test'
+// CHECK-B-NEXT: IsMethod:        true
+// CHECK-B-NEXT: Parent:
+// CHECK-B-NEXT:   Type:            Record
+// CHECK-B-NEXT:   Name:            'Struct'
+// CHECK-B-NEXT:   USR:             '0E0522198A2C7D141073719DA0815FCD6DC95610'
+// CHECK-B-NEXT: ReturnType:
+// CHECK-B-NEXT:   Type:
+// CHECK-B-NEXT:     Name:            'void'
+// CHECK-B-NEXT: ...
+
+
+class Class {
+ public:
+  void publicMethod();
+  int  publicField;
+ protected:
+  void protectedMethod();
+  int  protectedField;
+ private:
+  void privateMethod();
+  int  privateField;
+};
+
+
+// CHECK-C: ---
+// CHECK-C-NEXT: USR:             '7CDD73DCD6CD72F7E5CE25502810A182C66C4B45'
+// CHECK-C-NEXT: Name:            'Class'
+// CHECK-C-NEXT: DefLocation:
+// CHECK-C-NEXT:   LineNumber:      58
+// CHECK-C-NEXT:   Filename:        'test'
+// CHECK-C-NEXT: TagType:         Class
+// CHECK-C-NEXT: Members:
+// CHECK-C-NEXT:   - Type:
+// CHECK-C-NEXT:       Name:            'int'
+// CHECK-C-NEXT:     Name:            'publicField'
+// CHECK-C-NEXT: ...
+
+
+// CHECK-D: ---
+// CHECK-D-NEXT: USR:             '05682AF6307337176313391E00A231C2C55A4972'
+// CHECK-D-NEXT: Name:            'publicMethod'
+// CHECK-D-NEXT: Namespace:
+// CHECK-D-NEXT:   - Type:            Record
+// CHECK-D-NEXT:     Name:            'Class'
+// CHECK-D-NEXT:     USR:             '7CDD73DCD6CD72F7E5CE25502810A182C66C4B45'
+// CHECK-D-NEXT: Location:
+// CHECK-D-NEXT:   - LineNumber:      60
+// CHECK-D-NEXT:     Filename:        'test'
+// CHECK-D-NEXT: IsMethod:        true
+// CHECK-D-NEXT: Parent:
+// CHECK-D-NEXT:   Type:            Record
+// CHECK-D-NEXT:   Name:            'Class'
+// CHECK-D-NEXT:   USR:             '7CDD73DCD6CD72F7E5CE25502810A182C66C4B45'
+// CHECK-D-NEXT: ReturnType:
+// CHECK-D-NEXT:   Type:
+// CHECK-D-NEXT:     Name:            'void'
+// CHECK-D-NEXT: ...