[Lldb-commits] [PATCH] D58971: Move MemoryRegionInfo into the Utility module

Wed Mar 6 07:03:02 PST 2019

labath added a comment.

In D58971#1418843 <https://reviews.llvm.org/D58971#1418843>, @zturner wrote:

> One idea for a new module name could be `AbstractProcess`, but I can't think of anything else better at the moment.

Yeah, naming the new module might be the hardest problem here. :) I'd call the module `ProcessInfo`, but we already have a class with that name. :P `AbstractProcess` might be the next best thing.

So I tried to enumerate what files could possibly go into this new module, and I came up with this list (it's a bit shorter than I originally thought):

- Environment.h
- MemoryRegionInfo.h
- ProcessInfo.h
- RegisterValue.h
- State.h
- TraceOptions.h

They seem to be already self-contained (as in no other Utility file uses those), so the move should be fairly easy. `Args.h` could be added to the list if CompletionRequest if moved elsewhere, but that may not be a good idea, since `Args` is also used for the built-in interpreter, and not only for process arguments. `ProcessLaunchInfo` could go there once PseudoTerminal has been factored out of it.

In terms of the dependency graph, I guess this library would need to come between Utility (so it can use things like ArchSpec and FileSpec) and Host (so we can use it to describe Host processes). Right now that seems to be fine, but it may have implications for the future -- I think this excludes the idea of merging Host and Utility into a single module (which I'm personally fine with), as that would mean this whole chain would collapse.

So, what do you think? Is doing this now worth the trouble or do we wait and see? Is there any other class/file that might fit in here?

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58971/new/

https://reviews.llvm.org/D58971