This is an archive of the discontinued LLVM Phabricator instance.

Differential D114243

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

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

Details

Reviewers
• Quuxplusone
Mordante
Group Reviewers
Restricted Project
Commits
rGe0f58444e126: [libc++] Fix feature test macro for __cpp_lib_to_chars
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
Herald added a subscriber: libcxx-commits. · 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
ldionne added a comment.Nov 19 2021, 8:00 AM
In D114243#3142866, @Quuxplusone wrote:

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.

Herald added a subscriber: arichardson. · View Herald TranscriptNov 19 2021, 8:00 AM
• Quuxplusone accepted this revision as: • Quuxplusone.Nov 19 2021, 8:15 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.

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
Harbormaster completed remote builds in B135125: Diff 388507.Nov 19 2021, 9:54 AM
ldionne added a comment.Nov 19 2021, 11:26 AM
In D114243#3142923, @Quuxplusone wrote:

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

In D114243#3143089, @Mordante wrote:

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!

Closed by commit rGe0f58444e126: [libc++] Fix feature test macro for __cpp_lib_to_chars (authored by ldionne). · Explain WhyNov 19 2021, 11:26 AM
This revision was automatically updated to reflect the committed changes.
ldionne added a commit: rGe0f58444e126: [libc++] Fix feature test macro for __cpp_lib_to_chars.
Mordante added a comment.Nov 19 2021, 12:19 PM
In D114243#3143509, @ldionne wrote:
In D114243#3143089, @Mordante wrote:

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.)

Revision Contents

PathSize
libcxx/
test/
std/
language.support/
support.limits/
support.limits.general/

Diff 388492

libcxx/test/std/language.support/support.limits/support.limits.general/charconv.pass.cpp

  • This file was deleted.
//===----------------------------------------------------------------------===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//
//
// <charconv> feature macros
/* Constant Value
__cpp_lib_to_chars 201611L
*/
#include <charconv>
#include <cassert>
#include "test_macros.h"
int main(int, char**)
{
// ensure that the macros that are supposed to be defined in <utility> are defined.
/*
#if !defined(__cpp_lib_fooby)
# error "__cpp_lib_fooby is not defined"
#elif __cpp_lib_fooby < 201606L
# error "__cpp_lib_fooby has an invalid value"
#endif
*/
return 0;
}

libcxx/test/std/language.support/support.limits/support.limits.general/memory_resource.version.pass.cpp

  • This file was deleted.
//===----------------------------------------------------------------------===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//
//
// <memory_resource> feature macros
/* Constant Value
__cpp_lib_memory_resource 201603L
*/
// XFAIL
// #include <memory_resource>
#include <cassert>
#include "test_macros.h"
int main(int, char**)
{
// ensure that the macros that are supposed to be defined in <memory_resource> are defined.
/*
#if !defined(__cpp_lib_fooby)
# error "__cpp_lib_fooby is not defined"
#elif __cpp_lib_fooby < 201606L
# error "__cpp_lib_fooby has an invalid value"
#endif
*/
return 0;
}