[Lldb-commits] [PATCH] D18530: Move some functions from ClangASTContext to ClangUtil

[Tamas Berghammer via lldb-commits]

tberghammer added a subscriber: tberghammer.
tberghammer added a comment.

I don't maintain any downstream branch to worry about merges but my personal opinion is moving large amount of code around can cause some issue in the future even for upstream. The 2 main issue I can think about:

- "git log" and "git blame" will not display the original change for the code what is problematic when we try to track down why a piece of code have been added (especially for special cases)
- As an active LLDB developer I have a pretty good idea about where a given piece of code lives. Moving the code around invalidates this knowledge for every developer in the project

So in general if we get some actual advantage from moving the code (e.g. cleaner API, better testing options, etc...) then I have no issue around it but if the only gain is the file size reduction and the removal of the cyclic dependencies then I think this causes more problem then the benefit of the change (same reason why we don't run clang-format over the full code base).

In this concrete case I don't see the benefit neither in the API nor for testing as all functions were accessible before in the same way and the tests can call the functions from the ClangASTContext instead of the ClangUtils with the same functionality.


http://reviews.llvm.org/D18530