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