[Lldb-commits] [PATCH] D43837: Move option parsing out of the Args class

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In https://reviews.llvm.org/D43837#1022303, @jingham wrote:

> I don't have any objections to the contents of this patch per se.  But I wonder if having to do all this work to separate the uses of Args from the options parser so we don't drag it in in some low level uses doesn't rather mean the Args class was not appropriate for use at that level, when what they really meant was just a collection of strings.  For instance, we use the Args class to hold arguments to pass to running processes.  That's convenient when we parse them from commands into run-args, but hardly essential to the function of passing a collection of strings to posix_spawnp or its like.
>
> I haven't gone through and audited all the uses you are trying to separate out from the options part of Args, but if possible it would be cleaner to have the class that's supposed to be cheek to jowl with the command line parsing, and another to store a list of strings.


I was thinking about that as well, but eventually I chose this approach. The reason for that is that there is already functionality in the Args class that is useful for posix_spawn and friends, and that's the ability to turn itself into an argv vector. So, the new class couldn't just be a `vector<string>`, but it would need some additional smartness. So after implementing that, I think I would need to find a way to rip that code out of the Args class (maybe by making the new class a member of Args). So the end result may end up being very similar, we would just reach in it a different way.

The other reason I want to move this out is the single responsibility principle. Right now, I can identify about 4 responsibilities of the Args class:

- being a representation of a list of arguments (I put it as a separate item because of the argv requirement)
- parsing a string into a list of arguments
- parsing a list of arguments into options
- parsing a string into random other objects (via various static functions)

Now we probably don't want to go all out and split this into 4 classes, but I figured the first two items are enough for a single class (one could even argue the two items are actually a single thing). I think parsing a string into args and parsing args into options are two sufficiently complicated algorithms that makes sense to keep them separate.

The thing I'm not 100% clear on is whether the Options class is the best home for these functions. The reason I chose this at the end (instead of e,g, putting it in a new class) was because I saw this pattern in the CommandObject:

  options->NotifyOptionParsingStarting(&exe_ctx);
  ...
  options->Parse(...);
  ...
  options->NotifyOptionParsingFinished(&exe_ctx);

It seemed to be that having these functions in the Options class would open up possibilities for simplifying the Options interface by folding the `Notify` functions into the `Parse` call.


https://reviews.llvm.org/D43837