[PATCH] D39459: Rename SymbolBody -> Symbol.

George Rimar via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Nov 2 01:30:32 PDT 2017


grimar added a comment.

In https://reviews.llvm.org/D39459#912906, @ruiu wrote:

> I intentionally avoided renaming variables to make this a mechanical change. I'll rename variables later, but I don't want to do that in this patch.


I am sorry but I really do not see why we would want to land this patch as is. Current situation is that `SymbolBody` is used everywhere and you are trying to rename it
to `Symbol`. That is good. Currently each place is known to need renaming, but naming of variables is also known to be correct.
After mechanical change you end up with set of places where each of them either correct or not, it is
set of Shroedinger's cats. Until you look on each you never know if it is correct or not, so to fix it you anyways will need to search for all `Symbol` lines in code again 
and check for each place that code around uses proper variables naming. So I do not see how mechanical renaming helps to improve code.

I realize that doing such change manually is much more time consuming. And my suggestion is next. 
If you want I can prepare renaming patch manually for LLD-ELF part and send on review. 
It will be definetely better than this in my opinion at least because will fix obvious issues with naming things around changes.


https://reviews.llvm.org/D39459





More information about the llvm-commits mailing list