[PATCH] D48341: [clang-doc] Adding a second reduce pass

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.


================
Comment at: clang-tools-extra/clang-doc/Representation.h:173
   bool mergeable(const Info &Other);
+  llvm::Expected<Reference> getEnclosingScope();
 };
----------------
Comment? What is an enclong scope?


================
Comment at: clang-tools-extra/clang-doc/Representation.h:264
 
+// A standalone function to wrap a reference in an Info of the appropriate
+// enclosing scope.
----------------
"A standalone function" is redundant.

It's unclear to me what "wrap a reference in an Info of the appropriate enclosing scope" mean. How does this change the input, and what are the output?


================
Comment at: clang-tools-extra/clang-doc/tool/ClangDocMain.cpp:121
 llvm::Expected<llvm::SmallString<128>>
 getPath(StringRef Root, StringRef Ext, StringRef Name,
         llvm::SmallVectorImpl<doc::Reference> &Namespaces) {
----------------
This deserves a comment. It's hard to tell what this does without looking at the implementation. 


================
Comment at: clang-tools-extra/clang-doc/tool/ClangDocMain.cpp:147
 
+void addToOutput(
+    StringRef Key, std::unique_ptr<doc::Info> &&I,
----------------
`Output` is a bit vague here. It's usually used for output stream. Maybe `addToInfos`?


================
Comment at: clang-tools-extra/clang-doc/tool/ClangDocMain.cpp:238
+    
+    // Prepare for second reduce pass.
+    
----------------
Could you add a comment explaining what the second reduction is?


================
Comment at: clang-tools-extra/clang-doc/tool/ClangDocMain.cpp:238
+    
+    // Prepare for second reduce pass.
+    
----------------
ioeric wrote:
> Could you add a comment explaining what the second reduction is?
I think a lot of code here is too specific to be in main function. Can they live in a library? 


================
Comment at: clang-tools-extra/clang-doc/tool/ClangDocMain.cpp:249
+    if (EnclosingScope.get().RefType == doc::InfoType::IT_function)
+      continue;
+    
----------------
Are symbols declared in functions indexed by the tool? If not, this should probably assert or at least emit a warning message.


================
Comment at: clang-tools-extra/clang-doc/tool/ClangDocMain.cpp:252
+    std::string EnclosingScopeKey;
+    if (EnclosingScope.get().USR == doc::SymbolID())
+      EnclosingScopeKey = "";
----------------
What scopes would have empty USR? Could you add a comment?


================
Comment at: clang-tools-extra/clang-doc/tool/ClangDocMain.cpp:277
+      // Wrap a reference in the type of the enclosing scope.
+      std::unique_ptr<doc::Info> Ref = llvm::make_unique<doc::RecordInfo>(
+          Reduced.get()->USR, Reduced.get()->Name);
----------------
This does exactly the same thing as the case above except for the doc info type... could the code be shared?


================
Comment at: clang-tools-extra/clang-doc/tool/ClangDocMain.cpp:304
+    default:
+      llvm::errs() << "Error sorting for second reduce pass.\n";
+    }
----------------
How severe is this? Should the tool bail out?


================
Comment at: clang-tools-extra/clang-doc/tool/ClangDocMain.cpp:309
+  // Second reducing phase (by scope) and docs generation phase
+  llvm::outs() << "Further reducing " << ReduceOutput.size() << " infos...\n";
+  for (auto &Group : ReduceOutput) {
----------------
It would be helpful to print more messages. What kind of reduction is this?


================
Comment at: clang-tools-extra/test/clang-doc/yaml-comments.cpp:30
+// CHECK: ---  
+// CHECK-NEXT: USR:             '0000000000000000000000000000000000000000'
+// CHECK-NEXT: ChildFunctions:  
----------------
I wouldn't check the exact value of USR in the tests. It would be very painful to fix tests when USR semantics are updated. Maybe just check that this has the expected number of characters?

Same below.


https://reviews.llvm.org/D48341