Page MenuHomePhabricator

Move lib/Demangle under lib/Support. No functionality change intended.
AbandonedPublic

Authored by rsmith on Aug 16 2018, 4:07 PM.

Details

Summary

In preparation for making Demangle conditionally use functionality from
Support, combine the two components into one. The Demangle/ directory
hierarchy is maintained even though there's only one header in

include/llvm/Support/Demangle

for now. I'm intending to add more soon.

Diff Detail

Event Timeline

rsmith created this revision.Aug 16 2018, 4:07 PM

I removed the copy of Compiler.h from lib/Support/Demangle, because there is no longer any layering reason to not just use the version from include/llvm/Support. (libc++abi will still need its own Compiler.h.)
I did not remove the StringView.h from lib/Support/Demangle, because the StringView class has a different interface from both llvm::StringRef and std::string_view, and keeping that file around to provide the interface that the demangling code wants seems like the best path for now. (libc++abi presumably can't depend on std::string_view, so needs its own implementation anyway.)

Why keep the directory? I'd generally prefer not adding more directory layers and just using reasonable naming patterns.

If the licensing issue is that libc++abi can't include code from Support, then it also can't include Compiler.h. So I feel like this code move is just a bandaid instead of a proper solution. Can we use dependency injection or something to get some hooks into the demangler that allow you to customize the behavior from Clang / LLVM, as described in D50828?

I removed the copy of Compiler.h from lib/Support/Demangle, because there is no longer any layering reason to not just use the version from include/llvm/Support. (libc++abi will still need its own Compiler.h.)
I did not remove the StringView.h from lib/Support/Demangle, because the StringView class has a different interface from both llvm::StringRef and std::string_view, and keeping that file around to provide the interface that the demangling code wants seems like the best path for now. (libc++abi presumably can't depend on std::string_view, so needs its own implementation anyway.)

FWIW, I'd prefer that we make whatever libc++abi needs match the API of StringRef so that we can just have two modes for a single file, and leave the mini-StringRef-like thing in libc++abi....

If the licensing issue is that libc++abi can't include code from Support, then it also can't include Compiler.h.

libc++abi has its own Compiler.h both before and after this change, so I believe that's a non-issue.

Can we use dependency injection or something to get some hooks into the demangler that allow you to customize the behavior from Clang / LLVM, as described in D50828?

Potentially, yes, and Erik and I have discussed some ways to do that in the longer term. But I think the demangler belongs in Support regardless of whether or not we factor out that part.

I did not remove the StringView.h from lib/Support/Demangle, because the StringView class has a different interface from both llvm::StringRef and std::string_view, and keeping that file around to provide the interface that the demangling code wants seems like the best path for now. (libc++abi presumably can't depend on std::string_view, so needs its own implementation anyway.)

FWIW, I'd prefer that we make whatever libc++abi needs match the API of StringRef so that we can just have two modes for a single file, and leave the mini-StringRef-like thing in libc++abi....

I'm fine with that as a direction if the owners of the demangler are happy with it. I'd like to keep this particular patch focused on just the file moves, though. (Removing Compiler.h was cheap to do as part of the move because the interface was identical to that of the Support version.)

I did not remove the StringView.h from lib/Support/Demangle, because the StringView class has a different interface from both llvm::StringRef and std::string_view, and keeping that file around to provide the interface that the demangling code wants seems like the best path for now. (libc++abi presumably can't depend on std::string_view, so needs its own implementation anyway.)

FWIW, I'd prefer that we make whatever libc++abi needs match the API of StringRef so that we can just have two modes for a single file, and leave the mini-StringRef-like thing in libc++abi....

I'm fine with that as a direction if the owners of the demangler are happy with it. I'd like to keep this particular patch focused on just the file moves, though. (Removing Compiler.h was cheap to do as part of the move because the interface was identical to that of the Support version.)

My issue is that until that happens, we kind of have to put this in a separate directory within Support.

But I don't think that makes sense long-term, so it would seem useful to fix the StringView issue thing first, and then do the move. If fixing the StringView thing is really hard, then I would honestly prefer to name it DemanglerStringView.h or something so we don't have to add a directory component that will stop making sense later on...

I think we should 1) retain the directory structure in Support that we had in Demangle, and 2) keep StringView in LLVM (I'm fine with making the API consistent with string_view's or StringRef's though).

I want to do 2) because we're going to have to support a hand-rolled string_view in the demangler (until C++17) regardless, and it seems needlessly messy and error-prone to have ItaniumDemangle.cpp and cxa_demangle.cpp call into two different implementation of the same thing. Also, I'm worried about drive-by refactoring of LLVM APIs diverging the two demanglers.

I want to do 1) as well because I think it's cleaner to have the rule: "the Support/Demangle directory is mirrored in libcxxabi/src/Demangle" than the rule: "A handful of random files in Support related to demangling are mirrored in libcxxabi". It would be nice if the demangler could live as a first-class citizen of Support, but it doesn't, and I don't think we should lie about it.

I'm putting this on hold for now.

After https://reviews.llvm.org/D50930 lands, we can consider moving the LLVM-specific demangling pieces to Support/ so that Demangle/ is an LLVM-independent component from which a demangler can trivially be constructed. Then we can have Demangle/ be identical across in LLVM and libc++.

rsmith abandoned this revision.Oct 22 2018, 2:38 PM