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

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: include/lldb/Interpreter/Options.h:123-126
+  llvm::Expected<Args> Parse(const Args &args,
+                             ExecutionContext *execution_context,
+                             lldb::PlatformSP platform_sp,
+                             bool require_validation);
----------------
zturner wrote:
> labath wrote:
> > zturner wrote:
> > > It appears that all of these could be static functions.  Can we do that?
> > They can't be. All of them access the `this` object. If you look at the original functions, they were taking an `Options&` as an argument and `Args` as `this`. These have that inverted.
> I originally searched for `m_` and didn't find anything so assumed they could be static.  But it looks like they are calling member functions, which is why I didn't see it.  It's too bad it can't even be `const`, given that it returns a copy of the args.  Seems like an awkward interface, maybe future cleanup can try to tackle that though.  Anyway, ignore my comment.
The returning of args is kind of a side effect of the function. The return value contains the arguments that could not be parsed into options (more precisely, the positional arguments). The main effect of the function is that it populates the Options object with the options that have been parsed. (I should probably add that to the comment).


https://reviews.llvm.org/D43837