Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fs.filesystem.syn] Fixes related to chrono::file_clock usage in <filesystem> #2285

Merged
merged 2 commits into from Oct 9, 2018

Conversation

gamecentric
Copy link
Contributor

The synopsis of refers to the types chrono::time_point and chrono::file_clock, so it must include for their declarations
The removed paragraph refers to trivial-clock that has now been replaced by chrono::file_clock. The paragraph is now unnecessary since chrono::file_clock is fully described in [time.clock.file].
See also http://cplusplus.github.io/LWG/lwg-active.html#3145

…nd chrono::file_clock, so it must include <chrono> for their declarations

The removed paragraph refers to trivial-clock that has now been replaced by chrono::file_clock. The paragraph is now unnecessary since chrono::file_clock is fully described in [time.clock.file].
See also http://cplusplus.github.io/LWG/lwg-active.html#3145
@jwakely
Copy link
Member

jwakely commented Aug 2, 2018

Requiring <chrono> is a normative change, not editorial. It's not necessary (implementations would be required to ensure the necessary types are available, but that doesn't mean the whole of <chrono> has to be).

@gamecentric
Copy link
Contributor Author

gamecentric commented Aug 2, 2018

Would the rest of the proposed change qualify as editorial if I remove the inclusion of <chrono>?

@jwakely
Copy link
Member

jwakely commented Aug 2, 2018

I'm not sure if the rest is editorial even. Removing redundant wording that applies to an unspecified type that is no longer used is editorial, but I wonder if the wording about reflecting the OS-dependent resolution and range of file time values should be present on file_clock instead. If we remove that wording then nothing would be left to suggest that file_clock should be able to represent file timestamps.

@jensmaurer jensmaurer changed the title Fixes related to chrono::file_clock usage in <filesystem> [fs.filesystem.syn] Fixes related to chrono::file_clock usage in <filesystem> Aug 2, 2018
@jensmaurer
Copy link
Member

jensmaurer commented Aug 2, 2018

In [time.syn], there's a

template<class Duration>
using file_time = time_point<file_clock, Duration>;

which looks pretty close to file_time_type defined in [fs.filesystem.syn].
I also notice that the suggested change leaves the resolution/range wording in, applying to file_time_type, not to file_clock. Which probably is appropriate, since we then take the clock's duration type.
In short, the suggestion change of removing discussion of trivial-clock seems correct and editorial to me.

@jensmaurer
Copy link
Member

jensmaurer commented Aug 2, 2018

It would be good to add "that is capable of representing and measuring file time values." to the definition of file_clock in [time.clock.file.overview] p1, though. @zygoloid, is that still editorial?

@jensmaurer jensmaurer added the decision-required A decision of the editorial group (or the Project Editor) is required. label Aug 2, 2018
@gamecentric
Copy link
Contributor Author

We could suggest that change to [time.clock.file.overview] as part of the resolution of LWG issue 3145, which is currently under discussion on the reflector.

@zygoloid
Copy link
Member

zygoloid commented Oct 9, 2018

The old type was required to be "capable of representing and measuring file time values". The new type "should [have sufficient] resolution and range [to] reflect the operating system dependent resolution and range of file time values", which seems like a strictly weaker requirement -- and appears to be an intentional weakening.

(I would note that there are some Linux file systems that represent 128-bit file times, and we probably want to permit a 64-bit file time implementation regardless, so the existing "should" wording may well be the right thing. But whether it's right or not is up to LWG.)

That is to say: I'm merging this to remove the remaining vestige of a deleted type; retaining the "capable of representing and measuring file time values" and applying it to the new type would in my view not be editorial.

@zygoloid zygoloid merged commit ad6b6f2 into cplusplus:master Oct 9, 2018
@gamecentric gamecentric deleted the fix-fs.filesystem.syn branch February 11, 2019 08:42
@jensmaurer jensmaurer removed the decision-required A decision of the editorial group (or the Project Editor) is required. label Oct 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants