Page MenuHomePhabricator

Explicitly add language standard option to test cases that rely on the C++14 default
ClosedPublic

Authored by nemanjai on Feb 1 2019, 5:30 AM.

Details

Summary

One of the platforms on which we do regular builds has some system headers that are not compatible with C++14. As a result, we can't compile any code (including test-suite tests) if we leave the default as is. So what we do is set the CMake option that allows us to set clang's default language level (we set it to gnu++11 in our case). However, this causes failures in all of the lit tests attached.

This patch simply adds the -std=gnu++14 option to match the typical default for clang that the test cases rely on.

I've tried to add people that have added/modified the tests as reviewers to this patch - so I am sorry about the very long list of reviewers. Please let me know if this is an acceptable change.

Diff Detail

Repository
rC Clang

Event Timeline

nemanjai created this revision.Feb 1 2019, 5:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2019, 5:30 AM
Herald added a subscriber: eraman. · View Herald Transcript

This seems innocuous to me. I don't see any problem with it, but please don't commit until the others have had time to look.

The change looks reasonable to me, given that you can change the default language version via a build argument.
I don't have the required experience with the lit-tests to give proper approval, though, would wait for someone more relevant to do this.

Do have a comment, though. Any, reason to use -std=gnu++14 and not -std=c++14?
Most (all?) of the tests do not seem to have anything to do with the gnu extensions, so why enable them?

...
Do have a comment, though. Any, reason to use -std=gnu++14 and not -std=c++14?
Most (all?) of the tests do not seem to have anything to do with the gnu extensions, so why enable them?

I only used gnu++14 in the test cases to match the current default in clang (i.e. not change how clang handles the test case). But I have no problem whatsoever with making these standard C++.

I agree: if a test doesn't seem to require the GNU extensions, let's not use -std=gnu++XX. Otherwise this LGTM.

nemanjai updated this revision to Diff 185093.Feb 4 2019, 10:47 AM

Changed the option to standard C++ rather than GNU extensions.

ilya-biryukov accepted this revision.EditedFeb 5 2019, 2:13 AM

LGTM. I think it's fine to land it, since no-one raised any concerns so far.

This revision is now accepted and ready to land.Feb 5 2019, 2:13 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 5 2019, 4:08 AM