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