This is an archive of the discontinued LLVM Phabricator instance.

[SystemZ][z/OS] Avoid assumption for character value in futures tests
ClosedPublic

Authored by muiez on Aug 18 2021, 12:37 PM.

Details

Summary

The aim of this patch is to remove the assumption that the character 'a' is always 97. In turn, this patch explicitly uses the character values to account for the EBCDIC 'a' that is not 97.

Diff Detail

Event Timeline

muiez requested review of this revision.Aug 18 2021, 12:37 PM
muiez created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptAug 18 2021, 12:37 PM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript
Quuxplusone requested changes to this revision.Aug 18 2021, 8:46 PM
Quuxplusone added a subscriber: Quuxplusone.
Quuxplusone added inline comments.
libcxx/test/std/thread/futures/futures.task/futures.task.members/operator.pass.cpp
90

I suggest that a much nicer way to fix all these tests would be to eliminate the (int, char) signature and the cast-from-char-to-double entirely. Just have it take double(int, float) or something, and pass 4.0 instead of 'a'.

Slightly less invasive: Leave the (int, char) signature, but pass 97 instead of 'a'.

More invasive, and "clever," but clean-looking: Leave the signature, pass '0' instead of 'a', and check that the result is (double)'8'... no, wait, that would be really dumb. Don't do that. :)

Either way, please upload with full context: use git show -U999 or git diff -U999 or whatever the arc incantation is.

This revision now requires changes to proceed.Aug 18 2021, 8:46 PM
muiez updated this revision to Diff 367563.Aug 19 2021, 11:32 AM
muiez edited the summary of this revision. (Show Details)
muiez marked an inline comment as done.
muiez added inline comments.
libcxx/test/std/thread/futures/futures.task/futures.task.members/operator.pass.cpp
90

Thanks for the suggestion. Please see updated diff :)

Quuxplusone added inline comments.
libcxx/test/std/thread/futures/futures.task/futures.task.members/operator.pass.cpp
90

LGTM now. Of course it's now utterly bizarre that we're choosing magic numbers like 97, 99, 122 instead of, say, 1, 2, 3... but that's a pre-existing weirdness of these tests. I think this is a good solution at this point.

muiez marked 2 inline comments as done.Aug 19 2021, 12:43 PM

As a side note, any idea why the Group Reviewer status is still red although the revision was accepted? It says "Requested Changes to Prior Diff (by @Quuxplusone)"

Quuxplusone accepted this revision.Aug 19 2021, 6:29 PM
Quuxplusone added a subscriber: ldionne.

It's because I accepted as "Quuxplusone" but not as "libc++"; normally we like to get an OK from two libc++ reviewers (and/or @ldionne) before granting the "libc++" project approval.
Anyway, I think this is obviously fine, so while I encourage you to wait for someone else's rubber-stamp, I'm also going to just mark it accepted anyway, so even if nobody new looks at it you won't be blocked.

This revision is now accepted and ready to land.Aug 19 2021, 6:29 PM
muiez updated this revision to Diff 367779.Aug 20 2021, 6:47 AM

Using clang-format.

muiez added a comment.Aug 20 2021, 6:48 AM

It's because I accepted as "Quuxplusone" but not as "libc++"; normally we like to get an OK from two libc++ reviewers (and/or @ldionne) before granting the "libc++" project approval.
Anyway, I think this is obviously fine, so while I encourage you to wait for someone else's rubber-stamp, I'm also going to just mark it accepted anyway, so even if nobody new looks at it you won't be blocked.

Ohh I see. Thanks for the clarification, noted!