[PATCH] D41102: Setup clang-doc frontend framework

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

You can erase some namespace prefix in code, e.g. llvm or clang, because you are working in them and/or had `using namespace clang`. Imo this is not required only if you find it ok to shorten the code.



================
Comment at: tools/clang-doc/ClangDoc.cpp:30
+bool ClangDocVisitor::VisitNamedDecl(const NamedDecl *D) {
+  SourceManager &Manager = Context->getSourceManager();
+  if (!IsNewComment(D->getLocation(), Manager))
----------------
Here manager can be const& if it is on the other places too


================
Comment at: tools/clang-doc/ClangDoc.cpp:50
+void ClangDocVisitor::ParseUnattachedComments() {
+  SourceManager &Manager = Context->getSourceManager();
+  for (RawComment *Comment : Context->getRawCommentList().getComments()) {
----------------
I think Manager can be const&. Looks like only read methods were called.


================
Comment at: tools/clang-doc/ClangDoc.cpp:61
+bool ClangDocVisitor::IsNewComment(SourceLocation Loc,
+                                   SourceManager &Manager) const {
+  if (!Loc.isValid())
----------------
Manager could be const&.


================
Comment at: tools/clang-doc/ClangDoc.h:38
+
+  virtual bool VisitNamedDecl(const NamedDecl *D);
+
----------------
Not sure if this method should be virtual. `RecursiveASTVisitor` uses the crtp to not need virtual methods but still behaving the same.


================
Comment at: tools/clang-doc/ClangDocReporter.cpp:155
+  CurrentCI->SelfClosing = C->isSelfClosing();
+  if (C->getNumAttrs() != 0) {
+    for (unsigned i = 0, e = C->getNumAttrs(); i != e; ++i) {
----------------
The condition for this if might trigger unexpected behaviour if `getNumAttrs` returns negative values. To be safe you could use > 0 instead != 0


================
Comment at: tools/clang-doc/ClangDocReporter.cpp:170
+  CurrentCI->Name = getCommandName(C->getCommandID());
+  for (unsigned i = 0, e = C->getNumArgs(); i != e; ++i)
+    CurrentCI->Args.push_back(C->getArgText(i));
----------------
Similar here. If e is negative you will execute the loop until I wraps around. Not sure if this is a real issue, depending on the postcondition of getNumArgs.


================
Comment at: tools/clang-doc/ClangDocReporter.cpp:188
+  if (C->isPositionValid()) {
+    for (unsigned i = 0, e = C->getDepth(); i != e; ++i)
+      CurrentCI->Position.push_back(C->getIndex(i));
----------------
Similar thing.


================
Comment at: tools/clang-doc/ClangDocReporter.cpp:228
+  for (comments::Comment *Child :
+       llvm::make_range(C->child_begin(), C->child_end())) {
+    CommentInfo ChildCI;
----------------
Extract range into utility method of `Comment`


================
Comment at: tools/clang-doc/ClangDocReporter.cpp:249
+const char *ClangDocReporter::getCommandName(unsigned CommandID) {
+  ;
+  const CommandInfo *Info = CommandTraits::getBuiltinCommandInfo(CommandID);
----------------
Empty Statement


================
Comment at: tools/clang-doc/ClangDocReporter.h:51
+  llvm::SmallVector<int, 8> Position;
+  std::vector<CommentInfo> Children;
+};
----------------
Here a short `children()` method return llvm::make_range shortens the code in a later loop and might benefit in the future for iterations over children.


https://reviews.llvm.org/D41102