My vote is to err on the side of leaving stuff in Utility that is general (like SharingPtr) but only used by one client, rather than moving it next to its current only client. After all, you are likely to go dumpster diving in Utility when you need a tool, but you're less likely to look through the whole source base for this sort of thing. And we'd really like people to be drawing out of Utility whenever they can...
Jim > On Jan 31, 2017, at 4:37 PM, Zachary Turner <ztur...@google.com> wrote: > > Thanks for the clarification, I wrote "try to convert" there because I didn't > look closely enough to see what problem it was solving. Since it seems to be > specific to ValueObject (which a grep confirms), we may be able to just move > the code for SharingPtr next to ValueObject. That said, SharingPtr.h / .cpp > don't actually have any reverse dependencies, so we could also just leave it > alone. I mostly just put that on the list because if we intend for this to > be a general purpose library of reusable stuff, we might as well remove > things that are not seeing broad use. > > On Tue, Jan 31, 2017 at 4:32 PM Jim Ingham <jing...@apple.com> wrote: > > > On Jan 31, 2017, at 3:59 PM, Zachary Turner via Phabricator via > > lldb-commits <lldb-commits@lists.llvm.org> wrote: > > > > zturner created this revision. > > Herald added a subscriber: mgorny. > > > > The goal here is to make `lldbUtility` a library which depends on no other > > libraries. It seems like this library already exists in spirit as a place > > to house very low level code which is commonly used by all parts of LLDB, > > so it makes sense to designate this as the one top-level library. We can > > change the name in the future to something like `Support` if we choose to > > (implying that it is analogous to LLVMSupport), but for now I just want to > > break the dependencies. > > > > So, this patch deletes a bunch of dead code and moves some code that is > > actually only used in 1-2 places up to where it's actually used. > > > > This is not enough to get `Utility` dependency free. Further tasks that > > I've identified, but which are too large to fit into this patch, include: > > > > 1. Move `RegularExpression` from Core -> Utility (Long term: Delete and > > use LLVM) > > 2. Move `Stream` from Core -> Utility (Long term: Delete and use > > llvm::raw_ostream) > > 3. Move `ConstString` from Core -> Utility > > 4. Move `ProcessStructReader` from Utility -> Process > > 5. Move `RegisterNumber` from Utility -> Target > > 6. Move `Error` from Core -> Utility > > 7. Try to convert `ValueObject::GetSP()` from SharingPtr to > > `std::shared_ptr`, then delete `SharingPtr`. > > The problem SharingPtr is solving is that the value objects form a cluster: > the parent value object (representing some structure say) and any of it's > children (the subelement or the subelement of the subelement...) that have > been realized by the ChildAtIndex, ChildByPath, and Co. calls. And the way > the value objects work, the whole cluster has to stay alive as long as any of > the children are alive. That's because everybody just knows where they are > in their parent (as they should since they all can move around.) So you need > them all for jobs like recomputing values when the stop ID changes. But > users are not (and shouldn't be) required to manually hold onto any more than > the element they care about, which may very well be a deep child. SharingPtr > manages this cluster and makes sure it stays alive as long as any members of > the cluster are alive. > > I have no particular stake in how this is done, and if there's something > clever in llvm that can do the same job, I'm all for that. But you can't > replace it with a std::shared_ptr, that won't work. > > Jim > > > > 8. Move `ModuleCache` from Utility -> Target > > > > Finally, once all of those things are done, we can begin breaking up the > > `lldb-private.h`, and `lldb-enumerations.h`, etc header files and moving > > the enumerations to more appropriate locations, which will finally break > > this up. > > > > > > https://reviews.llvm.org/D29359 > > > > Files: > > lldb/include/lldb/Utility/ConvertEnum.h > > lldb/include/lldb/Utility/PriorityPointerPair.h > > lldb/include/lldb/Utility/Utils.h > > lldb/include/lldb/lldb-private-enumerations.h > > lldb/source/Commands/CommandObjectPlatform.cpp > > lldb/source/Core/Section.cpp > > lldb/source/Interpreter/OptionGroupArchitecture.cpp > > lldb/source/Interpreter/OptionGroupFormat.cpp > > lldb/source/Interpreter/OptionGroupOutputFile.cpp > > lldb/source/Interpreter/OptionGroupPlatform.cpp > > lldb/source/Interpreter/OptionGroupUUID.cpp > > lldb/source/Interpreter/OptionGroupValueObjectDisplay.cpp > > lldb/source/Interpreter/OptionGroupVariable.cpp > > lldb/source/Interpreter/OptionGroupWatchpoint.cpp > > lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp > > lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp > > lldb/source/Target/Platform.cpp > > lldb/source/Target/ThreadList.cpp > > lldb/source/Target/ThreadPlan.cpp > > lldb/source/Utility/ARM64_DWARF_Registers.cpp > > lldb/source/Utility/ARM64_DWARF_Registers.h > > lldb/source/Utility/ARM_DWARF_Registers.cpp > > lldb/source/Utility/ARM_DWARF_Registers.h > > lldb/source/Utility/CMakeLists.txt > > lldb/source/Utility/ConvertEnum.cpp > > > > <D29359.86516.patch>_______________________________________________ > > lldb-commits mailing list > > lldb-commits@lists.llvm.org > > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits > _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
