djasper added inline comments.
================ Comment at: lib/Format/WhitespaceManager.cpp:207 + + if (i != Start) { + if (Changes[i].NestingAndIndentLevel > ---------------- Merge the two ifs into a single one? ================ Comment at: lib/Format/WhitespaceManager.cpp:318 + for (unsigned e = Changes.size(); + i != e && Changes[i].NestingAndIndentLevel >= NestingAndIndentLevel; + ++i) { ---------------- I'd probably move the second condition into an early exit inside the loop: if (Changes[i].NestingAndIndentLevel >= NestingAndIndentLevel) break; ================ Comment at: lib/Format/WhitespaceManager.cpp:394 }, - Changes); + Changes, 0); } ---------------- Use a comment to describe the literal, i.e.: /*StartAt=*/0 ================ Comment at: lib/Format/WhitespaceManager.h:134 + // A combination of nesting level and indent level. + // The nesting level of this token, i.e. the number of surrounding (), + // [], {} or <>. Note that the value stored here is slightly different ---------------- This comment seems outdated. ================ Comment at: lib/Format/WhitespaceManager.h:217 + /// declaration name. + static bool IsStartOfDeclName(const FormatToken &Tok); + ---------------- This function should be a local function in the .cpp file. Also, can you describe in more detail what this does, i.e. what kind of declarations are covered here? Also, it is a bit confusing to have a function and a member of WhitespaceManager::Change with the exact same name. https://reviews.llvm.org/D21279 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits