Page MenuHomePhabricator

[CXX] Exercise all paths through these tests
ClosedPublic

Authored by probinson on Jun 27 2019, 1:29 PM.

Details

Summary

These 3 tests have dialect-based conditionals, but weren't running Clang with enough different dialects to actually enable those conditional sections.

Diff Detail

Repository
rL LLVM

Event Timeline

probinson created this revision.Jun 27 2019, 1:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 27 2019, 1:29 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Ping. This is pretty straightforward, the only question is whether we want to preserve these older-dialect tests or rip them out.

I would've assumed these conditionals were added by Sony folks for their change in default dialect - that doesn't necessarily mean these tests are needed upstream (the functionality may be tested elsewhere)

I would've assumed these conditionals were added by Sony folks for their change in default dialect - that doesn't necessarily mean these tests are needed upstream (the functionality may be tested elsewhere)

For two of the files, the conditionals were added by Richard Smith, not Sony. For the Sony case, it is the *older* path that was upstream originally, and that is the one that the tests stopped testing; there is no reason to think those paths are tested elsewhere.

there is no reason to think those paths are tested elsewhere.

Well that may be a tad strong. But I'm reluctant to remove them unless they are demonstrably tested elsewhere, and it's not clear to me how to look.

rsmith accepted this revision.Jul 9 2019, 12:38 PM
rsmith added inline comments.
clang/test/SemaCXX/linkage2.cpp
3 ↗(On Diff #206921)

Should this one be run in C++98 mode too? (The use of -Wno-c++11-extensions suggests to me that C++98 was expected.)

This revision is now accepted and ready to land.Jul 9 2019, 12:38 PM
probinson marked an inline comment as done.Jul 9 2019, 12:57 PM
probinson added inline comments.
clang/test/SemaCXX/linkage2.cpp
3 ↗(On Diff #206921)

I don't think so; -Wno-c++11-extensions will not suppress diagnostics about GNU extensions. I think GNU++98 is what's wanted here (and is the old default dialect).

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 9 2019, 1:48 PM