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