This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Fix feature test macro for __cpp_lib_to_chars
ClosedPublic

Authored by ldionne on Nov 19 2021, 6:55 AM.

Details

Summary

We would have been defining it in <utility> instead of <charconv>. For
the time being, this doesn't change anything since we don't implement
the feature test macro anyways.

Also, as a fly-by, this removes obsolete feature test macro tests. There
was a brief time back in the days when we wrote feature test macro tests
manually. In particular, we had test files for cpp_lib_to_chars and
cpp_lib_memory_resource. Since we now have a principled way of generating
these tests with scripts, this commit removes the obsolete (and empty)
tests for these two feature test macros.

Diff Detail

Event Timeline

ldionne requested review of this revision.Nov 19 2021, 6:55 AM
ldionne created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptNov 19 2021, 6:55 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone requested changes to this revision.Nov 19 2021, 7:54 AM
Quuxplusone added a subscriber: Quuxplusone.

This smelled fishy to me so I investigated a tiny bit. I believe we're defining __cpp_lib_to_chars in the wrong header (utility should be charconv). I didn't dig deeper than that, but I'd encourage you to. :)

This revision now requires changes to proceed.Nov 19 2021, 7:54 AM

This smelled fishy to me so I investigated a tiny bit. I believe we're defining __cpp_lib_to_chars in the wrong header (utility should be charconv). I didn't dig deeper than that, but I'd encourage you to. :)

You seem to be right (http://eel.is/c++draft/version.syn#lib:__cpp_lib_to_chars). Let me correct that.

But nonetheless, these manually written tests should go, do you agree?

ldionne updated this revision to Diff 388507.Nov 19 2021, 8:00 AM
ldionne retitled this revision from [libc++] Remove obsolete feature test macro tests to [libc++] Fix feature test macro for __cpp_lib_to_chars.
ldionne edited the summary of this revision. (Show Details)

Address Arthur's comments.

should go, do you agree?

Definitely, yes. You might do one more pass through wherever the feature-test macros are standardized, just to see if there's anything else that should be defined in <charconv> that we've missed.

Mordante accepted this revision.Nov 19 2021, 9:29 AM
Mordante added a subscriber: Mordante.

Nice catch. LGTM, I would be slightly concerned about removing it from the <utility> header if the feature were completely implemented.

This revision is now accepted and ready to land.Nov 19 2021, 9:29 AM

should go, do you agree?

Definitely, yes. You might do one more pass through wherever the feature-test macros are standardized, just to see if there's anything else that should be defined in <charconv> that we've missed.

That's the only FTM in <charconv> AFAICT: http://eel.is/c++draft/version.syn

Nice catch. LGTM, I would be slightly concerned about removing it from the <utility> header if the feature were completely implemented.

Yeah, agreed, luckily it has never been defined.

Thank you both!

This revision was automatically updated to reflect the committed changes.

Nice catch. LGTM, I would be slightly concerned about removing it from the <utility> header if the feature were completely implemented.

Yeah, agreed, luckily it has never been defined.

Well I have to disagree with that assessment, I would have loved us to have completed C++17 support by now.
(/me will try to complete to_chars float real soon now.)