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 <ztur...@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 > >
_______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
