[Lldb-commits] [PATCH] D18530: Move some functions from ClangASTContext to ClangUtil
Zachary Turner via lldb-commits
lldb-commits at lists.llvm.org
Wed Mar 30 10:32:32 PDT 2016
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
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
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
> 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).
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the lldb-commits