This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Optimize away unneeded length calculation in basic_string::compare(const char*)
ClosedPublic

Authored by EricWF on Aug 25 2015, 11:54 PM.

Diff Detail

Event Timeline

EricWF updated this revision to Diff 33183.Aug 25 2015, 11:54 PM
EricWF retitled this revision from to [libcxx] Optimize away unneeded length calculation in basic_string::compare(const char*).
EricWF updated this object.
EricWF added a reviewer: mclow.lists.
EricWF added a subscriber: cfe-commits.
mclow.lists edited edge metadata.Aug 26 2015, 12:48 PM

I think I'd rather see the call to strcmp and wcscmp in the char_traits class.

Won't this do the wrong thing for embedded '\0' in a std::string?

std::string("hello\0world", 11).compare("hello")

should not return 0.

Won't this do the wrong thing for embedded '\0' in a std::string?

std::string("hello\0world", 11).compare("hello")

should not return 0.

Woops. Yep that seems correct but it will sure hamper the possibility for optimization.

Won't this do the wrong thing for embedded '\0' in a std::string?

std::string("hello\0world", 11).compare("hello")

should not return 0.

Good point; I think that pretty much kills this proposed change.

EricWF updated this revision to Diff 33252.Aug 26 2015, 2:37 PM
EricWF edited edge metadata.

Instead of optimizing away the length calculation, this patch still performs it but uses the calculated length to return early if the two strings differ in length.

EricWF updated this revision to Diff 33253.Aug 26 2015, 2:38 PM

Fix debug assertion.

mclow.lists added inline comments.Aug 27 2015, 7:48 AM
include/string
3801

If the lengths are the same (and they are), you don't need the min of them

3803

Also, you should not call traits::compare here. You should call __rhs.compare(0, npos, __lhs, __lhs_len)

which is what __rhs.compare(__lhs) does.

EricWF marked an inline comment as done.Aug 27 2015, 3:45 PM
EricWF added inline comments.
include/string
3803

I actually don't want to invoke compare because it has different semantics and does unneeded work. compare first does a comparison using traits::compare and then does a comparison using length. We have already compared based on length and we don't need to do any of the extra work that compare(...) does. I think calling __rhs.compare(0, npos, __lhs, __lhs_len) would be a pessimization.

EricWF updated this revision to Diff 33368.Aug 27 2015, 3:46 PM

Remove unnecessary call to std::min.

EricWF updated this revision to Diff 33380.Aug 27 2015, 5:06 PM

Call basic_string::compare as requested by @mclow.lists.

The first change LGTM. The second one needs to match it.

include/string
3816

You'll want to do the same as above here.

mclow.lists accepted this revision.Aug 27 2015, 7:58 PM
mclow.lists edited edge metadata.
This revision is now accepted and ready to land.Aug 27 2015, 7:58 PM
EricWF closed this revision.Aug 27 2015, 8:03 PM