mclow.lists added a comment.
Howard just pointed out to me that `__clear_and_shrink` should be noexcept -
otherwise we get the generation of an exception table and a call to `terminate`
in string's move assignment operator. Fixed in revision 333435.
Repository:
rCXX libc++
https://reviews.
vsk added a comment.
Thanks @tvanslyke and @mclow.lists, landed as r327064.
Repository:
rCXX libc++
https://reviews.llvm.org/D41976
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rCXX327064: Low-hanging fruit optimization in
string::__move_assign(). (authored by vedantk, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D41976?vs=130819&id=137647#toc
Repository:
mclow.lists added a comment.
In https://reviews.llvm.org/D41976#1031674, @vsk wrote:
> @mclow.lists is this still fine to commit? I can land it and watch the bots,
> if you'd like.
Sure. Please do so.
https://reviews.llvm.org/D41976
___
cfe-comm
vsk added a comment.
Herald added a subscriber: christof.
@mclow.lists is this still fine to commit? I can land it and watch the bots, if
you'd like.
https://reviews.llvm.org/D41976
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lis
tvanslyke added a comment.
Hello. I would just like to follow up on the status of this patch. Is there
anything else that is needed from me? I just want to make sure that nobody is
waiting on me for anything; I'm still not 100% familiar with the process.
Thanks in advance.
https://reviews
tvanslyke updated this revision to Diff 130819.
tvanslyke added a comment.
Moved `__clear_and_shrink()` to live with the other public dunder methods and
adapted the associated test accordingly.
https://reviews.llvm.org/D41976
Files:
include/string
test/libcxx/strings/basic.string/string.m
mclow.lists added a comment.
> Since __clear_and_shrink() is private
There's no reason it has to be private.
People aren't supposed to call anything with a reserved name.
https://reviews.llvm.org/D41976
___
cfe-commits mailing list
cfe-commits@list
mclow.lists added a comment.
In https://reviews.llvm.org/D41976#982978, @tvanslyke wrote:
> I don't have commit access myself so I've added the test to the diff.
I'll commit it then.
https://reviews.llvm.org/D41976
___
cfe-commits mailing list
cf
tvanslyke updated this revision to Diff 130775.
tvanslyke added a comment.
Since `__clear_and_shrink()` is private the test covers copy and move
assignment. I also ran the libcxx/strings/basic.string and std/strings tests
with a hard-coded `assert(__invariants());` at the end of
`__clear_and_s
mclow.lists accepted this revision.
mclow.lists added a comment.
This revision is now accepted and ready to land.
This looks good to me.
Please add a test in test/libcxx/strings/string.modifiers and commit.
Something like this:
#include
#include
int main () {
std::string l = "Th
tvanslyke updated this revision to Diff 129969.
tvanslyke added a comment.
Implemented changes to ensure string state is valid after calling
`__clear_and_shrink()`. Benchmark results are identical.
https://reviews.llvm.org/D41976
Files:
string
Index: string
===
tvanslyke added a comment.
Sorry if I'm spamming too much. Just to be clear, I agree completely with your
sentiment but I'm pretty sure that `__clear_and_shrink()` has exactly the same
effects as `clear(); shrink_to_fit();`. Maybe `reserve(0);` should should be
unconditionally calling `__set_
tvanslyke added a comment.
Just to elaborate, in `reserve(0)` after the deallocation there's a check `if
(__now_long)` which takes the else branch and only ends up calling
`__set_short_size(__sz); // __sz = 0 here`. So even without this diff the only
thing `reserve(0)` does after deallocating
tvanslyke added a comment.
I think the only thing we need is a call to __set_long_cap(0) in the body of
the if() to make the string be in a valid state, but if you follow the logic
for a call to reserve(0) (after having called clear()) it doesn't even end up
doing that (unless I'm missing somet
mclow.lists added a comment.
I'm a bit leery of this patch. Not because of what it's trying to do, but
rather, the introduction of a method `__clear_and_shrink` that leaves the
string in an invalid state. For all the uses that you put it to, I don't
think that's a problem (though I'm still w
tvanslyke added a comment.
I can adapt it to the style guide if it is decided that it should be added.
https://reviews.llvm.org/D41976
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
lebedev.ri added a comment.
In https://reviews.llvm.org/D41976#975659, @tvanslyke wrote:
> Here are the results:
>
> Comparing old-copymove.json to new-copymove.json
> BenchmarkTime CPU Time Old
> Time New CPU Old CPU New
>
> -
tvanslyke added a comment.
I'll add that the reason I felt compelled to submit this change was because
perf was saying most of my program, that only ever moved strings, was spending
a majority of it's time in string::reserve() calls. If for no other reason, I
think it's confusing for people to
tvanslyke added a comment.
Here are the results:
Comparing old-copymove.json to new-copymove.json
BenchmarkTime CPU Time Old Time
New CPU Old CPU New
-
mclow.lists added a comment.
Can you share your benchmark results, please?
https://reviews.llvm.org/D41976
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
vsk added reviewers: EricWF, mclow.lists, hiraditya.
vsk added a comment.
Adding some folks who may be interested.
https://reviews.llvm.org/D41976
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo
tvanslyke updated this revision to Diff 129685.
tvanslyke added a comment.
I went ahead and just pulled it out to a small inline member function and added
it to __copy_assign_alloc(). Removed some other redundancies.
https://reviews.llvm.org/D41976
Files:
string
Index: string
tvanslyke created this revision.
tvanslyke added a reviewer: howard.hinnant.
Herald added a subscriber: cfe-commits.
shrink_to_fit() ends up doing a lot work to get information that we already
know since we just called clear(). This change seems concise enough to be
worth the couple extra lines
24 matches
Mail list logo
