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

[Greg Clayton via lldb-commits]

clayborg added a comment.

In http://reviews.llvm.org/D18530#387175, @zturner wrote:

> One or two of the functions, you are right.  They do `clang::ASTContext` related things.  That was actually an oversight on my part.  I meant to move only functions that specifically did NOT do `clang::ASTContext` things.  Like `RemoveFastQualifiers`, or converting enums, or converting from qual types to canonical qual types.  Very simple utility functions.


They are still clang::ASTContext related things since clang::QualType is part of that whole infrastructure.

> The reason why I don't think they should be in `ClangASTContext` from a design point of view is because they are used from all over the places, including from places that aren't actually holding and don't need an `ASTContext`.


Does it just come down to QualType stuff? Those are fine the exist in ClangUtil.cpp, but again, we have a ton or merges we are doing daily and any moving of code does cause merge conflicts when they are just moved to be logically moved elsewhere. When we have a fix, it first goes into llvm.org SVN, then we cherry pick it over to swift.org into two branches: master and master-next, then we cherry pick these fixes over to our internal branches where we at least have two more merges. Each merge is possibly going into older code where we would not have ClangUtil.cpp in that branch. So then we need to be very careful with our merges to make sure we do them right. We finally got to a place after splitting using DWARFASTParser and all that where merges are going very smoothy most of the time since anything language specific is off in a separate file.

> What if I move the `clang::ASTContext` functions back but keep the truly independent ones here?  `ClangASTContext` is a really egregiously oversized file, so I think splitting it up a bit would be helpful.  I also find it helpful to break a lot of these cyclic dependencies, where A.cpp includes B.h and B.cpp includes A.h, and we're doing that all over the place in large part because B.cpp is `ClangASTContext` and A.cpp is anything else that has to do anything, no matter what it is, to a clang type, even if it doesn't need an `ASTContext`.


We can probably simplify our header includes. The only time you need to include a header file is if it affects the layout of the class in question. That means base classes and instance variables only. Any functions that take full fledged types like:

  class Bar
  {
      void Foo(lldb_private::FileSpec file_spec);
  };

Doesn't require FileSpec.h, just a forward declaration of FileSpec, because it doesn't affect the layout of the class. Anyone that includes the above header might need to manually include "FileSpec.h", but we can probably simplify our includes quite a bit, so that might help.

So again, moving anything can cause merging headaches. See below for more details.

> What kind of merge conflict does it create?  There's no tricky changes here, why wouldn't they be mergeable to the other branch?  I'm not really crazy about the idea of having decisions about upstream being based on someone's downstream issues though :-/  If something is good for the upstream it seems like that's merit enough to get it in.


So one example is the RemoveFastQualifiers() fix. If we merge that back into a branch that doesn't have ClangUtil.cpp, we need to take the contents from any functions in ClangUtils.cpp and find the old corresponding functions and paste the new code back into there.

So if we only had private branches at Apple I wouldn't be making this argument. But on swift.org, we have public merges going on all the time with auto mergers. They can often fail when we do things like this (move stuff for organization sake), so then we have to manually intervene. There are 3 branches on swift.org that are always merging from other places. The LLVM and Clang on swift.org doesn't track top of tree SVN llvm and clang, but might be up to two months behind.

So I really implore us to not make these kinds of disruptive changes as it ends up causing our automation steps to fail and causes us to have to manually intervene and spend a lot of time doing manual merges.


http://reviews.llvm.org/D18530