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
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
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
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
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
-
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
12 matches
Mail list logo
