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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
