[PATCH] D41102: Setup clang-doc frontend framework

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere requested changes to this revision.
JDevlieghere added inline comments.
This revision now requires changes to proceed.


================
Comment at: tools/clang-doc/ClangDocReporter.cpp:106
+  FI.Filename = Filename;
+  FileRecords.insert(std::pair<StringRef, FileRecord>(Filename, std::move(FI)));
+}
----------------
JonasToth wrote:
> `std::make_pair(FileName, std::move(FI))` would be shorter.
Also I don't think there's a point in moving StringRefs as they are intended to be light weight already.


================
Comment at: tools/clang-doc/ClangDocReporter.cpp:136
+  }
+  C->isSelfClosing() ? CurrentCI->SelfClosing = true
+                     : CurrentCI->SelfClosing = false;
----------------
Why not `CurrentCI->SelfClosing = C->isSelfClosing()`?


================
Comment at: tools/clang-doc/ClangDocReporter.cpp:154
+      ParamCommandComment::getDirectionAsString(C->getDirection());
+  C->isDirectionExplicit() ? CurrentCI->Explicit = true
+                           : CurrentCI->Explicit = false;
----------------
Same here.


================
Comment at: tools/clang-doc/ClangDocReporter.cpp:205
+  ConstCommentVisitor<ClangDocReporter>::visit(C);
+  for (comments::Comment::child_iterator I = C->child_begin(),
+                                         E = C->child_end();
----------------
JonasToth wrote:
> `comments::Comment` could get a `childs()` method returning a view to iterate with nice range based loops.
Yup, you can use `llvm::make_range` for this.


================
Comment at: tools/clang-doc/ClangDocReporter.h:45
+  bool Explicit = false;
+  std::vector<std::string> Args;
+  std::vector<StringPair> Attrs;
----------------
Would this be a good use case for `llvm::SmallVector`?


================
Comment at: tools/clang-doc/ClangDocReporter.h:46
+  std::vector<std::string> Args;
+  std::vector<StringPair> Attrs;
+  std::vector<int> Position;
----------------
Why not use `llvm::StringMap` here? I'm guessing you went with a `StringPair` because of the YAMP serialization, but that should work with StringMap. 


================
Comment at: tools/clang-doc/ClangDocReporter.h:90
+
+  std::set<std::string> GetFilesInThisTU() const { return FilesInThisTU; }
+  bool HasFile(StringRef Filename) const;
----------------
You probably want to return a const-ref here, rather than a copy. 


https://reviews.llvm.org/D41102