This revision was automatically updated to reflect the committed changes.
Closed by commit rL283157: Refactor the Args class. (authored by zturner).
Changed prior to commit:
https://reviews.llvm.org/D25099?vs=73335&id=73418#toc
Repository:
rL LLVM
https://reviews.llvm.org/D25099
Files:
ll
Thanks, I'll fix it up before submitting
On Mon, Oct 3, 2016 at 2:40 PM Jim Ingham wrote:
> jingham added a comment.
>
> You messed up the meaning of one comment (noted inline). Otherwise this
> looks fine to me too.
>
>
>
> > Args.cpp:97-98
> > + // Argument can be split into multiple disconti
jingham added a comment.
You messed up the meaning of one comment (noted inline). Otherwise this looks
fine to me too.
> Args.cpp:97-98
> + // Argument can be split into multiple discontiguous pieces, for example:
> + // "Hello " "World"
> + // this would result in a single argument "Hell
tfiala accepted this revision.
tfiala added a comment.
This revision is now accepted and ready to land.
That works fine.
LGTM.
https://reviews.llvm.org/D25099
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/m
tfiala added a comment.
In https://reviews.llvm.org/D25099#559701, @zturner wrote:
> I know what this is. It should be fixed in this patch, I guess I didn't have
> the newest patch uploaded.
Okay, I'll give that a shot now.
https://reviews.llvm.org/D25099
zturner updated this revision to Diff 73335.
zturner added a comment.
I know what this is. It should be fixed in this patch, I guess I didn't have
the newest patch uploaded.
https://reviews.llvm.org/D25099
Files:
include/lldb/Interpreter/Args.h
source/Core/Logging.cpp
source/Core/String
tfiala requested changes to this revision.
tfiala added a reviewer: tfiala.
tfiala added a comment.
This revision now requires changes to proceed.
I'm getting one test crash (segfault) in logging/TestLogging.py:
Exception Type:EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: KERN_INVAL
tfiala added a comment.
I will test this on macOS. I will have the results this afternoon.
https://reviews.llvm.org/D25099
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Yep I plan on doing that.
-Todd
> On Oct 3, 2016, at 10:29 AM, Zachary Turner wrote:
>
> He lgtm'ed my last patch, so I guess he's ok with the general concept.
> Perhaps if someone could just run the test suite for me that would be good
> enough.
>
>> On Mon, Oct 3, 2016 at 10:25 AM Todd Fi
He lgtm'ed my last patch, so I guess he's ok with the general concept.
Perhaps if someone could just run the test suite for me that would be good
enough.
On Mon, Oct 3, 2016 at 10:25 AM Todd Fiala wrote:
> tfiala added a comment.
>
> @zturner , Greg is out this week (and was last Friday as well)
tfiala added a comment.
@zturner , Greg is out this week (and was last Friday as well).
I'll get somebody over here to review.
https://reviews.llvm.org/D25099
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/m
zturner added a comment.
Hi Greg, might you have a chance to look at this today? I've got a huge
backlog of CLs to get in. The rest probably won't require reviews, but this
one is a precursor to everything else.
https://reviews.llvm.org/D25099
_
zturner added inline comments.
> labath wrote in Args.cpp:207
> I think we don't need to call Clear() here, as all memory owned by the object
> will be correctly destroyed anyway.
Thanks for pointing that out. Originally I wasn't using `unique_ptr` but
rather `new[]` and `delete[]`. After I
labath added a comment.
random drive-by. Otherwise, I like it.
> Args.cpp:207
> +//--
> +Args::~Args() { Clear(); }
> +
I think we don't need to call Clear() here, as all memory owned by the object
will be correctly destroyed
zturner updated this revision to Diff 73095.
https://reviews.llvm.org/D25099
Files:
include/lldb/Interpreter/Args.h
source/Core/StringList.cpp
source/Interpreter/Args.cpp
unittests/Interpreter/TestArgs.cpp
Index: unittests/Interpreter/TestArgs.cpp
zturner updated this revision to Diff 73099.
zturner added a comment.
Renamed `EntryData` to `ArgEntry` and made it public. This will be useful
later for providing iterator support over the entries.
https://reviews.llvm.org/D25099
Files:
include/lldb/Interpreter/Args.h
source/Core/StringL
amccarth added a comment.
Just a drive by.
> Args.h:449
>//--
>// Classes that inherit from Args can see and modify these
>//--
This comment is no longer
zturner created this revision.
zturner added a reviewer: clayborg.
zturner added subscribers: lldb-commits, LLDB.
In https://reviews.llvm.org/D24952 I tried to refactor the `Args` class to get
rid of `m_argv`. The original motivation for this is that I wanted a way to
implement `GetArgumentAtIn
18 matches
Mail list logo
