omtcyfz added a comment. In https://reviews.llvm.org/D23279#510061, @alexshap wrote:
> First of all, many thanks for the comments & proposal of clang-refactor. > > 1. Regarding high-level project organization: clang-refactor - that can be a > good place for various refactoring techniques like the existing clang-rename, > my simple tool, etc - essentially what Kirill said in the e-mail. I would be > happy to join. At the same time - if i can make some progress before this > reorganization is introduced - that would be helpful, if not - no problem at > all. > 2. Regarding clang-tidy - it seems to me, that putting this particular tool > into PaddingChecker/clang-tidy is far from being perfect: A. In some cases > the optimal fields order is not unique, it would be more flexible to let the > user choose which order of fields he prefers. B. In some cases someone might > want to change the order of fields even if the padding is not affected at > all. C. Other concerns mentioned above in your comments I would love to have a check, which reorders stuff while optimising it! Though, clang-tidy is limited to a single TU, too, which is not good. Cheers! I now have a relevant example of what is refactoring and what is "addressing issues". So, 1. Optimising struct size is clearly "addressing issue". That's a good thing to have. 2. Reordering fields in struct is clearly refactoring. That's also a good thing to have. First thing is a **issue addressing** task. I would love to have such tool to run over my whole codebase automatically from times to times. As for the second tool - it performes a refactoring action by a user request, it has nothing common with **addressing issues**, which is what `clang-tidy` is meant for. > One more thing: > i am not sure i understood the comment > "It actually breaks code ... only works while it is in the same TU". > For instance if we have the following project: > /include/Point.h (contains the definition of the struct Point) > /a.cpp (uses Point) > /b.cpp (also uses Point) > /main.cpp (also uses Point) > clang-reorder-fields -i -record-name ::Point -fields-order y,x main.cpp > a.cpp b.cpp > works for me. Because everything is within a single Translation Unit :) Translation Unit is not equivalent to a file. > At the same time i see in my code the following issue (easy to fix): > it will complain here : > > if (auto Err = Replacements[R.getFilePath()].add(R)) > llvm::errs() << "Failed to add replacement, file: " << R.getFilePath() > > because the same replacement for the code in header is being added multiple > times, > but actually it doesn't affect the final set of changes and the result is > correct (i think we need either to handle this case more carefully or > (temporarily) add FIXME and replace llvm::errs with llvm::warns ). Repository: rL LLVM https://reviews.llvm.org/D23279 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
