Page MenuHomePhabricator

[libcxx] disables ranges for clang-cl
ClosedPublic

Authored by cjdb on Apr 23 2021, 4:09 PM.

Details

Reviewers
ldionne
zoecarver
Group Reviewers
Restricted Project
Commits
rGa224bf8ec423: [libcxx] disables ranges for clang-cl
Summary

clang-cl doesn't properly handle concepts right now and is failing CI.

Diff Detail

Event Timeline

cjdb requested review of this revision.Apr 23 2021, 4:09 PM
cjdb created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptApr 23 2021, 4:09 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
zoecarver accepted this revision.Apr 23 2021, 4:26 PM

Once the tests pass, please land this to fix the CI (that's why I'm approving as libc++ as well).

libcxx/include/__config
851

Will this catch "normal" msvc too?

This revision is now accepted and ready to land.Apr 23 2021, 4:26 PM
This revision was automatically updated to reflect the committed changes.
mstorsjo added inline comments.
libcxx/include/__config
851

Yes, it will. Currently, building libc++ with normal msvc isn't supported at all. Building libc++ with clang-cl but calling it from normal msvc probably is affected though, but I'm not sure how common that is. If needed we can make it more precise with && defined(__clang__) later.

Wouldn't it be better to properly add a libcpp-no-ranges to utils/libcxx/test/features.py?
I expect the something like this untested code the do the job:
Feature(name='libcpp-no-ranges', when=lambda cfg: featureTestMacros(cfg).get('__cpp_concepts', 0) < 201907) and _isMSVC and _isClang,

Then change the test from // UNSUPPORTED: libcpp-no-concepts to // UNSUPPORTED: libcpp-no-ranges and remove the XFAILs.

Wouldn't it be better to properly add a libcpp-no-ranges to utils/libcxx/test/features.py?
I expect the something like this untested code the do the job:
Feature(name='libcpp-no-ranges', when=lambda cfg: featureTestMacros(cfg).get('__cpp_concepts', 0) < 201907) and _isMSVC and _isClang,

... or (_isMSVC and _isClang) I suppose?

Then change the test from // UNSUPPORTED: libcpp-no-concepts to // UNSUPPORTED: libcpp-no-ranges and remove the XFAILs.

I guess that could work too, and would be less clutter. We won't notice when/if it gets fixed then though, but I don't think that's a big deal (as we'll have to minimize the bug and report it, so we'll probably have decent track of when/how it gets fixed anyway).

Wouldn't it be better to properly add a libcpp-no-ranges to utils/libcxx/test/features.py?
I expect the something like this untested code the do the job:
Feature(name='libcpp-no-ranges', when=lambda cfg: featureTestMacros(cfg).get('__cpp_concepts', 0) < 201907) and _isMSVC and _isClang,

... or (_isMSVC and _isClang) I suppose?

Yes.

Then change the test from // UNSUPPORTED: libcpp-no-concepts to // UNSUPPORTED: libcpp-no-ranges and remove the XFAILs.

I guess that could work too, and would be less clutter. We won't notice when/if it gets fixed then though, but I don't think that's a big deal (as we'll have to minimize the bug and report it, so we'll probably have decent track of when/how it gets fixed anyway).

True, but IMO it gives less clutter. If we want to see when it's fixed we could add a dummy test contain one test that currently fails. This test then uses

// UNSUPPORTED: libcpp-no-concepts
// XFAIL: msvc && clang

This test then should also contain coment to update utils/libcxx/test/features.py's libcpp-no-ranges lambda, once the test passes.
WDYT?

I guess that could work too, and would be less clutter. We won't notice when/if it gets fixed then though, but I don't think that's a big deal (as we'll have to minimize the bug and report it, so we'll probably have decent track of when/how it gets fixed anyway).

True, but IMO it gives less clutter. If we want to see when it's fixed we could add a dummy test contain one test that currently fails. This test then uses

// UNSUPPORTED: libcpp-no-concepts
// XFAIL: msvc && clang

This test then should also contain coment to update utils/libcxx/test/features.py's libcpp-no-ranges lambda, once the test passes.
WDYT?

That sounds good to me!