<div dir="ltr">Any issues with this?  Honestly you can probably just read the description and lgtm it based on that, because as I said it's 99% a code move, and there is no functional change except for the one bug fix in ClangUtil::RemoveFastQualifiers.<div><br></div><div>I have a followup patch almost complete which moves some more function, and adds some more unit tests, and (so far) fixes 2 more bugs that were found as a result of the unit tests.  So I think the work is a win just on the grounds that I've already found 3 bugs as a result, even disregarding the benefits to ClangASTContext in file size.</div><div><br></div><div>Let me know if I can continue this effort without review, it would save all of us some time and obviously I can use my judgement if I think the change is non-trivial.</div></div><br><div class="gmail_quote"><div dir="ltr">On Mon, Mar 28, 2016 at 4:32 PM Zachary Turner <<a href="mailto:zturner@google.com">zturner@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">zturner created this revision.<br>
zturner added reviewers: spyffe, clayborg.<br>
zturner added a subscriber: lldb-commits.<br>
<br>
This is pretty mechanical, so you don't really have to look at this in too much detail.  I'm mostly posting it here to make sure people are ok with the high level idea.  Basically ClangASTContext is a monstrous file and I want to reduce the size by splitting out some of the functionality.  There's a ton of functions in `ClangASTContext` that are static and are basically helper functions, but then other people like `DWARFASTParserClang`, or `ClangASTImporter`, or other places re-use those functions.  So the idea is just to move all these common clang helper functions into a single place.<br>
<br>
In doing so, it makes testing this type of functionality very easy, because you can write a unit test for every function in the file.  I did that in this patch, and actually found one bug as a result of my unittest failing (yay for unit tests).  Since that is the only functional change in the patch, you may want to look at it specifically.  It's `ClangUtil::RemoveFastQualifiers`.  The version before my patch did nothing, it returned exactly the same value it was passed in, because `QualType::getQualifiers()` returns a `clang::Qualifiers` by value, so it was not modifying the `QualType`'s qualifiers, but a copy of them, which was immediately discarded.<br>
<br>
If anyone can think of a way to exercise this in a public API test, let me know.<br>
<br>
I'm hoping to gradually move some more of the `ClangASTContext` functions over to `ClangUtil` in subsequent patches.  It makes understanding the important parts of `ClangASTContext` easier, and it will allow me to add more unittests for the rest of the functions as well (hopefully turning up more bugs).<br>
<br>
<a href="http://reviews.llvm.org/D18530" rel="noreferrer" target="_blank">http://reviews.llvm.org/D18530</a><br>
<br>
Files:<br>
  include/lldb/Symbol/ClangASTContext.h<br>
  include/lldb/Symbol/ClangUtil.h<br>
  source/API/SBTarget.cpp<br>
  source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp<br>
  source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp<br>
  source/Symbol/ClangASTContext.cpp<br>
  source/Symbol/ClangASTImporter.cpp<br>
  source/Symbol/ClangUtil.cpp<br>
  unittests/CMakeLists.txt<br>
  unittests/Symbol/CMakeLists.txt<br>
  unittests/Symbol/TestClangUtil.cpp<br>
<br>
</blockquote></div>