[Lldb-commits] [PATCH] D29359: Start breaking some dependencies in lldbUtility

[Jim Ingham via lldb-commits]

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 <zturner at 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 <jingham at apple.com> wrote:
> 
> > On Jan 31, 2017, at 3:59 PM, Zachary Turner via Phabricator via lldb-commits <lldb-commits at 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 at lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
>