This is an archive of the discontinued LLVM Phabricator instance.

[libc++][test] Adjust move_iterator tests to allow C++20
ClosedPublic

Authored by CaseyCarter on May 4 2020, 11:02 AM.

Details

Summary

These tests fail due to a couple of changes to move_iterator for C++20:

  1. move_iterator<I>::operator++(int) returns void in C++20 if I doesn't model forward_iterator.
  2. move_iterator<I>::reference is calculated in C++20, so I must actually have an operator*() const.

Diff Detail

Event Timeline

CaseyCarter created this revision.May 4 2020, 11:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 4 2020, 11:02 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

I'm not going to reformat *only* my additions per the clang-format instructions - that would be silly - and I suspect that folks would clang-format all of the tests if they actually wanted them formatted, so I won't format the entire file, either.

I'm not going to reformat *only* my additions per the clang-format instructions - that would be silly - and I suspect that folks would clang-format all of the tests if they actually wanted them formatted, so I won't format the entire file, either.

I'm not sure how the libcxx/ maintainers feel, but clang-formatting just the changes is the operating procedure in llvm/ and clang/. Here's a script that makes it easy:
https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting
I believe there's also a git-clang-format script checked in that you can leverage by adding it to your PATH.

The rules change regularly in subtle ways, usually because of fixes/regressions in clang-format, sometimes due to changes in policy. We don't do a mass reformat of the entire codebase on every commit though, instead we just update the lines that are being changed, edging things "closer". Maybe it would be better to do a mass reformat on policy changes but we haven't done that historically.

If a file is drastically out-of-whack, it's an option to reformat it in a separate format-only commit before making a change (never the same commit as a change because that makes it hard to see what changed).

Not trying to impose rules on libc++, just providing some context; I suspect these new linters are broadly following the procedure from LLVM and Clang.

libcxx/test/std/iterators/predef.iterators/move.iterators/move.iterator/types.pass.cpp
37–39

You seem to be changing from 2-space indent (the usual practice for LLVM) to 4-space indent here. I don't know if that's intentional, but if it's the right thing to do please do so in a separate commit to avoid adding noise to this patch.

clang-format new/changed sections of code per policy.

CaseyCarter marked 2 inline comments as done.May 6 2020, 4:02 PM

Format issues addressed.

libcxx/test/std/iterators/predef.iterators/move.iterators/move.iterator/types.pass.cpp
37–39

Intentional - 4-space is the predominant indent in this file - but misguided. Thank you for the rundown of general formatting policy.

ldionne accepted this revision.May 7 2020, 8:53 AM

Regarding the formatting changes, I personally think we should clang-format all of libc++, libc++abi and libunwind in one go. Then we'd be done with these small issues that add up. And we can even add the revision to .git-blame-ignore-revs.

This revision is now accepted and ready to land.May 7 2020, 8:53 AM
CaseyCarter marked an inline comment as done.May 7 2020, 9:56 AM

Regarding the formatting changes, I personally think we should clang-format all of libc++, libc++abi and libunwind in one go. Then we'd be done with these small issues that add up. And we can even add the revision to .git-blame-ignore-revs.

I'm the furthest thing from a core libc++ maintainer, but FWIW I agree. I set up a .git-blame-ignore-revs for the MSVC internal git repo a week or two ago with our "clang-format the things" and "change all the line endings" commits and it has been a great experience.

This revision was automatically updated to reflect the committed changes.

Regarding the formatting changes, I personally think we should clang-format all of libc++, libc++abi and libunwind in one go. Then we'd be done with these small issues that add up. And we can even add the revision to .git-blame-ignore-revs.

(FWIW, that's what I think we should do with llvm/ and clang/ too...)