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

[Zachary Turner via lldb-commits]

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.

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.

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.

On Mon, Mar 28, 2016 at 4:32 PM Zachary Turner <zturner at google.com> wrote:

> zturner created this revision.
> zturner added reviewers: spyffe, clayborg.
> zturner added a subscriber: lldb-commits.
>
> 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.
>
> 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.
>
> If anyone can think of a way to exercise this in a public API test, let me
> know.
>
> 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).
>
> http://reviews.llvm.org/D18530
>
> Files:
>   include/lldb/Symbol/ClangASTContext.h
>   include/lldb/Symbol/ClangUtil.h
>   source/API/SBTarget.cpp
>   source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
>   source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
>   source/Symbol/ClangASTContext.cpp
>   source/Symbol/ClangASTImporter.cpp
>   source/Symbol/ClangUtil.cpp
>   unittests/CMakeLists.txt
>   unittests/Symbol/CMakeLists.txt
>   unittests/Symbol/TestClangUtil.cpp
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20160330/3a070c3d/attachment.html>