This is an archive of the discontinued LLVM Phabricator instance.

Fix fpos requirements & cleanup
AbandonedPublic

Authored by zoecarver on Apr 9 2019, 5:35 PM.

Details

Reviewers
mclow.lists
howard.hinnant
EricWF
ldionne
Group Reviewers
Restricted Project
Summary

This patch implements P0759R1.

Event Timeline

zoecarver created this revision.Apr 9 2019, 5:35 PM
zoecarver updated this revision to Diff 194426.Apr 9 2019, 5:37 PM

Update www

EricWF requested changes to this revision.Apr 9 2019, 10:03 PM

I don't really understand how a lot of the changes in this patch implement the changes in the paper. Can you explain the rationale for each change?

The tests for each individual change need to be put in the exiting test files for the operations they modify.

I think this patch needs some work.

include/string
557 ↗(On Diff #194426)

fpos<_StateT> is the same as fpos in this context, no?

559 ↗(On Diff #194426)

This seems wrong. It's just another copy constructor overload, but that takes by value.

test/std/strings/fpos/fpos.pass.cpp
1 ↗(On Diff #194426)

These tests should be integrated into the existing tests files under std/input.output/iostreams.base/fpos/fpos.operations.

The test name and directory correspond directly to the stable name of the section of the standard it is testing.

33 ↗(On Diff #194426)

No test for the trivial copy constructor requirements?

This revision now requires changes to proceed.Apr 9 2019, 10:03 PM
zoecarver updated this revision to Diff 194864.Apr 12 2019, 6:29 AM

I got confused about what type o was in the paper. This patch should conform to the paper properly. I will update the tests in the correct places but wanted to get this part out for review first.

zoecarver marked an inline comment as done.Apr 12 2019, 6:32 AM
zoecarver added inline comments.
include/string
553 ↗(On Diff #194864)

The overloads conflict with the plus and minus overloads for streamoff. I think it would be fine to use those overloads (because they will convert both sides to streamoff), but I am also happy to define the behavior explicitly.

zoecarver updated this revision to Diff 199950.May 16 2019, 5:49 PM
  • update & add tests
  • update synopsis
  • clean up members / operators
zoecarver marked an inline comment as done.May 16 2019, 5:52 PM
zoecarver added inline comments.
include/string
529 ↗(On Diff #199950)

I re-ordered and updated these operators and member functions. I don't think it changes any of the functionality so I am fine without the updates (say the word, and I will remove the changes).

zoecarver updated this revision to Diff 199952.May 16 2019, 5:59 PM
  • remove unneeded assignment operator
  • move tests to correct location

(sorry for the repetitive updates)

mclow.lists added inline comments.May 22 2019, 7:35 AM
include/string
529 ↗(On Diff #199950)

I think that's a good idea. This paper is supposed to be just wording cleanup; no changes to functionality, so lets make this patch just a test cleanup.

The re-ordering, etc of the member functions and operators can be done afterwards as a NFC patch.

zoecarver marked an inline comment as done.May 22 2019, 7:40 AM
zoecarver added inline comments.
include/string
529 ↗(On Diff #199950)

Sounds good. I will remove this part of the patch and re-upload the diff.

zoecarver updated this revision to Diff 200754.May 22 2019, 8:18 AM
  • remove changes to fpos
mclow.lists added inline comments.May 23 2019, 9:21 AM
test/std/input.output/iostreams.base/fpos/fpos.operations/fpos.pass.cpp
20 ↗(On Diff #200754)

Why is this unsupported? fpos was a thing in C++98/03.

34 ↗(On Diff #200754)

error: static_assert with no message is a C++17 extension

zoecarver marked an inline comment as done.May 23 2019, 5:13 PM
zoecarver added inline comments.
test/std/input.output/iostreams.base/fpos/fpos.operations/fpos.pass.cpp
20 ↗(On Diff #200754)

But the updates made here were not a thing until C++20. Should I still have the test for earlier versions? Or should I change this to only work after C++17?

ldionne requested changes to this revision.Oct 22 2019, 4:10 PM

Some of the tests you added are already present in other files, for example libcxx/test/std/input.output/iostreams.base/fpos/fpos.operations/subtraction.pass.cpp. Any reason why you're duplicating those checks in test/std/input.output/iostreams.base/fpos/fpos.operations/fpos.pass.cpp?

test/std/input.output/iostreams.base/fpos/fpos.operations/fpos.pass.cpp
20 ↗(On Diff #200754)

Since this is a DR, I think it's OK to assume the fix is there in prior standards. So you should remove the UNSUPPORTED annotation.

28 ↗(On Diff #200754)

std::declval<T const&>() instead?

34 ↗(On Diff #200754)

@zoecarver Action item: add a message!

54 ↗(On Diff #200754)

I don't think you need any other types.

This revision now requires changes to proceed.Oct 22 2019, 4:10 PM

Yes, some of the tests are duplicated (subtraction, addition, comparison, etc.) but, I think it is nice to have it match the order of the table in the standard. Say the word and I'll remove the duplication, though.

zoecarver updated this revision to Diff 226669.Oct 28 2019, 8:34 AM
zoecarver retitled this revision from Fix fpos requirements & cleanup to Fix fpos requirements & cleanup.
  • fix review comments
Herald added a project: Restricted Project. · View Herald TranscriptOct 28 2019, 8:34 AM
Herald added a subscriber: christof. · View Herald Transcript
ldionne accepted this revision.May 7 2020, 10:13 AM
Herald added a reviewer: Restricted Project. · View Herald TranscriptMay 7 2020, 10:13 AM
ldionne added inline comments.May 7 2020, 10:14 AM
libcxx/www/cxx2a_status.html
96

Can you mark it as being implemented in LLVM 11.0?

These tests should be integrated into the existing tests files under std/input.output/iostreams.base/fpos/fpos.operations.

@EricWF Do you still want me to split this up? I'm going to commit this patch as-is (with the change @ldionne asked for) but, if you still want me to split this test up, post a comment here and I'll revert, make the change, and put the patch here for re-review.

Committed as df73e36dc6ffd2ede500189b075f7ac52794b3ad. I can't close the revision for some reason (probably because of the blocking reviews).

zoecarver abandoned this revision.May 7 2020, 2:24 PM