This revision was automatically updated to reflect the committed changes.
Closed by commit rL329677: Move Args::StringTo*** functions to a new
OptionArgParser class (authored by labath, committed by ).
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
labath added a comment.
Thank you.
PS: I will need someone to update the XCode project after this.
https://reviews.llvm.org/D44306
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
jingham accepted this revision.
jingham added a comment.
This revision is now accepted and ready to land.
Yes, with your updated naming, I am fine with this.
https://reviews.llvm.org/D44306
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
labath added a comment.
Jim, judging by the comment in https://reviews.llvm.org/D44306#1037587 you were
fine with this patch except the naming part. Now that that's fixed, I hope this
should be fine. Unless I hear from you, I am going to commit this so I can
continue with re-layering the
labath added a comment.
So, any objections to this patch?
https://reviews.llvm.org/D44306
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
labath updated this revision to Diff 138540.
labath added a comment.
s/toAddress/ToAddress/
https://reviews.llvm.org/D44306
Files:
include/lldb/Interpreter/Args.h
include/lldb/Interpreter/OptionArgParser.h
include/lldb/Interpreter/Options.h
source/API/SBDebugger.cpp
labath added a comment.
I agree with Jim. I deliberately put here only the types that are used for
command parsing, since I presume these are the things that the
Command/Interpreter modules will depend on (it's not fully clear to me where to
draw the line between these two, so it may end up
jingham added a comment.
As long as these functions are used for parsing options args in individual
CommandObject implementations, and don't get dragged lower in the stack, I
don't think that's a problem. By the time you're getting to individual
commands you are on top of pretty much
zturner added a subscriber: labath.
zturner added a comment.
I think having all parsing functions in a single place will just move the
layering problem elsewhere, since a bunch of conversion functions for
objects from various libraries will be mushed together into one place.
I think having all parsing functions in a single place will just move the
layering problem elsewhere, since a bunch of conversion functions for
objects from various libraries will be mushed together into one place.
On Wed, Mar 14, 2018 at 11:34 AM Jim Ingham via Phabricator <
jingham added a comment.
Except for the to -> To to keep consistent with all the other lldb function
naming this looks fine.
Now that they are all together it's easy to see we haven't been consistent in
these functions. We really should make ToFormat return the format & take an
error
labath updated this revision to Diff 138418.
labath added a comment.
Herald added a subscriber: mgorny.
This is a version that moves the StringTo*** functions to a new
"OptionArgParser" class. I'm not terribly proud of the name, but I couldn't
think of anything better -- we already have a
labath planned changes to this revision.
labath added a comment.
Yes, that thought occurred to me almost immediately after I submitted this.
I'll try to implement something like that instead.
Since this will be just a bunch of static functions with no state, I think we
don't even have to put
labath created this revision.
labath added reviewers: jingham, zturner.
StringToAddress could end up evaluating an expression, which is a somewhat
unexpected behavior of a seemingly simple method. I rename it to
EvaluateAddressExpression and move it to the Target class, next to the standard
14 matches
Mail list logo
