jolesiak marked 6 inline comments as done.
jolesiak added a comment.

In https://reviews.llvm.org/D48716#1146796, @benhamilton wrote:

> > Count selector parts also for method declarations.
>
> What bug does this fix? Can you add a test which breaks before this change 
> and is fixed by this change?


Sorry for the confusion.

General comment to changes https://reviews.llvm.org/D48716, 
https://reviews.llvm.org/D48718, https://reviews.llvm.org/D48719, 
https://reviews.llvm.org/D48720 (which are the split of 
https://reviews.llvm.org/D48352):
These changes are separate, in the sense that they fix different issues. 
However, they should be chained as every change (apart from the first one) is 
based on previous ones: https://reviews.llvm.org/D48716 -> 
https://reviews.llvm.org/D48718 -> https://reviews.llvm.org/D48719 -> 
https://reviews.llvm.org/D48720. I don't know how to chain them in Phabricator 
(I should probably leave some comments about what every change is based on 
though).

https://reviews.llvm.org/D48716 fixes the mechanism of counting parameters. 
This is an internal change though and doesn't influence formatting on its own 
(at the current state). Its lack would be visible after applying 
https://reviews.llvm.org/D48719. Therefore, I'm not aware of any formatting 
test that fails before applying this change and succeeds after. As far as I 
know internal functions/methods are not tested at all. Thus, the tests are part 
of https://reviews.llvm.org/D48719.

This is why initially I put everything into one change: 
https://reviews.llvm.org/D48352, but I agree that it was not readable at all.


Repository:
  rC Clang

https://reviews.llvm.org/D48716



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to