[PATCH] D23279: clang-reorder-fields

[Kirill Bobyrev via cfe-commits]

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