Page MenuHomePhabricator

[SystemZ][z/OS] enable trigraphs by default on z/OS
ClosedPublic

Authored by abhina.sreeskantharajan on Aug 11 2020, 5:02 AM.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptAug 11 2020, 5:02 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
abhina.sreeskantharajan requested review of this revision.Aug 11 2020, 5:02 AM
abhina.sreeskantharajan retitled this revision from [z/OS] enable trigraphs by default on z/OS to [SystemZ][z/OS] enable trigraphs by default on z/OS .Aug 11 2020, 5:11 AM
clang/lib/Frontend/CompilerInvocation.cpp
2787–2789

If going with an integrated update to the existing sentences, both of the original sentences would require an update (not just the first one). The first sentence is also meant to describe GCC's behavior, for which I am not sure there's a statement to be made for GCC on z/OS. I suggest to add an additional sentence instead.

2791

I would like to point out that processing trigraphs when most platforms don't would be a portability concern. Clang appears to mitigate this somewhat with warnings.

clang/test/Frontend/trigraphs.cpp
8

Do we know if -fno-trigraphs is meaningfully functional on z/OS? I believe trigraph usage might need to be replaced to use digraphs in the system headers before using -fno-trigraphs can be expected to work in a real user application.

fanbo-meng requested changes to this revision.Aug 13 2020, 6:40 AM
fanbo-meng added a subscriber: fanbo-meng.

Can you modify clang/test/Frontend/trigraphs.cpp and clang/test/Lexer/cxx1z-trigraphs.cpp ? These tests are assuming trigraphs are disabled by -std=gnu++11 -fms-compatibility -std=c++1z, and it won't be the case here because on z/OS trigraphs are still enabled when these options are present.

This revision now requires changes to proceed.Aug 13 2020, 6:40 AM

Thanks Hubert and Fanbo for reviewing. I updated the comment to Hubert's suggestion, and updated both testcases as requested.

abhina.sreeskantharajan marked 3 inline comments as done.Aug 13 2020, 7:07 AM
abhina.sreeskantharajan added inline comments.
clang/lib/Frontend/CompilerInvocation.cpp
2791

Unfortunately we can't move away from this because the system headers currently use trigraphs. But we will definitely look into whether we can update these headers to not be dependent on trigraphs and improve portability.

clang/test/Frontend/trigraphs.cpp
8

With the current system headers, compiling with -fno-trigraphs would not work in a user application. As mentioned above, we will look into whether we can remove this dependency.

fanbo-meng accepted this revision.Aug 13 2020, 7:12 AM
This revision is now accepted and ready to land.Aug 13 2020, 7:12 AM
abhina.sreeskantharajan marked 2 inline comments as done.Aug 13 2020, 7:15 AM
clang/test/Lexer/cxx1z-trigraphs.cpp
31

This strategy duplicates the trigraphs-enabled behaviour checks three times.

Plain -verify can be used along with a macro instead (without such duplication). This also allows the explicit -fno-trigraphs case to be added with little cost.

Pseudo-code:

// RUN: ...
// RUN: ... -ftrigraphs -DENABLED_TRIGRAPHS=1
// RUN: ... -fno-trigraphs -DENABLED_TRIGRAPHS=0

#ifdef __MVS__
#ifndef ENABLED_TRIGRAPHS
#define ENABLED_TRIGRAPHS 1
#endif
#endif

#if !ENABLED_TRIGRAPHS
// ...
#else
// ...
#endif

Thanks Hubert for the suggestion. I've updated the lit test.

abhina.sreeskantharajan marked an inline comment as done.Aug 13 2020, 9:23 AM

LGTM with suggested changes.

clang/test/Lexer/cxx1z-trigraphs.cpp
15

Intentional whitespace disappeared.

22

This seems to have changed in a weird way?

Thanks for catching that. I fixed up the testcase.

abhina.sreeskantharajan marked 2 inline comments as done.Aug 13 2020, 10:55 AM