Skip to content

Commit

Permalink
Don't assume that char_traits::compare returns +/-1 (#225)
Browse files Browse the repository at this point in the history
  • Loading branch information
@vitaut
vitaut committed Nov 10, 2015
1 parent 8b86a74 commit aa741ba
Showing 1 changed file with 2 additions and 2 deletions.
4 changes: 2 additions & 2 deletions test/util-test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -724,8 +724,8 @@ void CheckOp() {

TEST(UtilTest, StringRefCompare) {
EXPECT_EQ(0, StringRef("foo").compare(StringRef("foo")));
EXPECT_EQ(1, StringRef("fop").compare(StringRef("foo")));
EXPECT_EQ(-1, StringRef("foo").compare(StringRef("fop")));
EXPECT_GT(StringRef("fop").compare(StringRef("foo")), 0);
EXPECT_LT(StringRef("foo").compare(StringRef("fop")), 0);
EXPECT_EQ(1, StringRef("foo").compare(StringRef("fo")));
EXPECT_EQ(-1, StringRef("fo").compare(StringRef("foo")));
CheckOp<std::equal_to>();
Expand Down

8 comments on commit aa741ba

@LarsGullik
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you have the same problem on the next lines as well? (729, 730)

@vitaut
Copy link
Contributor Author

@vitaut vitaut commented on aa741ba Nov 10, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. Unlike char_traits::compare, the size comparison always returns +/-1: https://github.com/cppformat/cppformat/blob/477962884e36ae490f2414df44dbc60abcba1ff2/format.h#L313

@LarsGullik
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok... OTOH

you could do:

result = size_ - other.size_;

instead of the branching/comparisons. On my box that is 2 vs ~7 instructions.

@LarsGullik
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My suggestion above would not work correctly with very large strings though, but is what libstdc++ does
in essence. To handle huge strings you would have to add the comparions again.

@vitaut
Copy link
Contributor Author

@vitaut vitaut commented on aa741ba Nov 10, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this is not portable because size_t can be larger than int. It is possible to check this at compile time and use size_ - other.size_ when possible, but it's probably an overkill.

@vitaut
Copy link
Contributor Author

@vitaut vitaut commented on aa741ba Nov 10, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My suggestion above would not work correctly with very large strings though, but is what libstdc++ does
in essence. To handle huge strings you would have to add the comparions again.

Right. Another option is to return a signed counterpart of size_t form compare instead of int, but again, I don't think it's worth the trouble.

@LarsGullik
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right.
However I still thing your tests should not test for -1,1 explicit. IMHO better to do you did and compare
againt < > 0 in all cases.

@vitaut
Copy link
Contributor Author

@vitaut vitaut commented on aa741ba Nov 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. Removed the +/-1 assumption in d6d019a. Thanks for the feedback.

Please sign in to comment.