Fixing buffer_appender's ++ slicing. #1822
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
brevzin
commented
Aug 14, 2020
| buffer_appender tmp = *this; | ||
| ++*this; | ||
| return tmp; | ||
| } |
There was a problem hiding this comment.
Given that back_insert_iterator only has like... 4 member functions, and I've already implemented 2 of them (and over-implemented them too, these could both just be return *this;), maybe we should just implement the whole thing in here without std?
vitaut
requested changes
Aug 18, 2020
test/format-test.cc
Outdated
| @@ -2473,3 +2473,37 @@ TEST(FormatTest, FormatUTF8Precision) { | |||
| EXPECT_EQ(result.size(), 5); | |||
| EXPECT_EQ(from_u8str(result), from_u8str(str.substr(0, 5))); | |||
| } | |||
|
|
|||
| #ifdef __cpp_decltype_auto | |||
There was a problem hiding this comment.
Can we make the test work without this, e.g. checking if the types are the same manually?
There was a problem hiding this comment.
Done. Just have a static_assert.
test/format-test.cc
Outdated
Comment on lines
2478
to
2481
| struct lazy_optional { | ||
| bool engaged; | ||
| std::string value; | ||
| }; |
There was a problem hiding this comment.
I'd minimize this even further into an empty struct and make the name reflect what we are trying to test here.
| buffer_appender tmp = *this; | ||
| ++*this; | ||
| return tmp; | ||
| } |
vitaut
reviewed
Aug 18, 2020
test/format-test.cc
Outdated
| auto format(check_back_appender, Context& ctx) -> decltype(ctx.out()) { | ||
| // needs to satisfy weakly_incrementable | ||
| auto out = ctx.out(); | ||
| static_assert(std::is_same<decltype(++out), decltype(out)&>::value); |
There was a problem hiding this comment.
Please add a message to fix contbuild.
|
Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
buffer_appender's pre- and post-fix increment operators slice down toback_inserter, which means it actually breaks the iterator requirements (which would be actually checked in C++20, it is notweakly_incrementable) and also breaks any code that assumes that++itandithave the same type (as illustrated in the test case, a reduced form of how we format our optional type, which no longer compiles).