ymandel marked 2 inline comments as done. ymandel added a comment. In D56786#1359920 <https://reviews.llvm.org/D56786#1359920>, @lebedev.ri wrote:
> In D56786#1359903 <https://reviews.llvm.org/D56786#1359903>, @ymandel wrote: > > > In D56786#1359879 <https://reviews.llvm.org/D56786#1359879>, @steveire > > wrote: > > > > > Can you break this up into multiple commits? > > > > > > Sure, but any suggestions on granularity? E.g. i can split into two: the > > fixes/clarifications in one and the new matcher in another; or i could > > split into four -- one for each bullet, etc. > > > Each matcher separately would be best, you then end up with 3x NFC doc-only > changes, and 2x matchers. Once I'm splitting, I'd prefer to bundle the tests with the corresponding comment changes, but I see how that would break the NFC split (assuming tests count as "functional changes"). So, if the NFC changes are separate, should all the tests be bundled together, or split into separate patches? For example, I can put the tests for existing matchers into one patch, and the new matcher and its test into a separate patch. WDYT? ================ Comment at: include/clang/ASTMatchers/ASTMatchers.h:3346 + +/// Overloaded to match the type's declaration. +AST_MATCHER_P_OVERLOAD(clang::CXXMemberCallExpr, invokedAtType, ---------------- lebedev.ri wrote: > How is this different from the other one? > Presumably it should have it's own docs. I was following what we've done elsewhere for overloads (e.g. line 3312 above), but am happy to expand this if you recommend. Personally, I think more documentation is better, but wanted to stick w/ the standard practice. ================ Comment at: include/clang/ASTMatchers/ASTMatchers.h:3350-3353 + using ::clang::ast_matchers::on; + using ::clang::ast_matchers::anyOf; + using ::clang::ast_matchers::hasType; + using ::clang::ast_matchers::pointsTo; ---------------- lebedev.ri wrote: > I don't think these are needed. > You don't have them in the matcher above yet it presumably compiles. Right -- those are leftover from development (which took place in a different namespace originally). Will remove once I split this patch. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56786/new/ https://reviews.llvm.org/D56786 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
