Page MenuHomePhabricator

[clang] tests: cleanup, update and add some new ones
ClosedPublic

Authored by mizvekov on Mar 23 2021, 4:38 PM.

Details

Summary

This reworks a small set of tests, as preparatory work for implementing
P2266.

  • Run for more standard versions, including c++2b.
  • Normalize file names and run commands.
  • Adds some extra tests.

New Coroutine tests taken from Aaron Puchert's D68845.

Signed-off-by: Matheus Izvekov <mizvekov@gmail.com>

Diff Detail

Event Timeline

mizvekov created this revision.Mar 23 2021, 4:38 PM
mizvekov requested review of this revision.Mar 23 2021, 4:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 23 2021, 4:38 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

IMHO you should just go ahead and land the trivial trailing-whitespace changes.

clang/test/CXX/dcl.dcl/dcl.spec/dcl.type/dcl.spec.auto/p7-cxx14.cpp
3

(1) You skipped 17.
(2) Isn't there some way to mark a Clang test as "run this in all modes '14 and later"? We shouldn't have to touch every single test every 3 years; there must be a way to do this; I just don't know what it is.

clang/test/CXX/drs/dr3xx.cpp
6

cxx98_2b is "unconditional", right?

444

This gives an error and a C++20 warning? 😛
Oh yeah. Huh. https://godbolt.org/z/eMY9zMs54 That's definitely a Clang bug. Feel like filing it?

clang/test/CXX/temp/temp.decls/temp.mem/p5.cpp
8

And 11, 14, 17?

clang/test/SemaCXX/constant-expression-cxx11.cpp
3

And 14, 17?

1770

What are those trailing (occasionally doubled) backslashes doing on lines 1768–1770?
Get rid of them.
You might then need to say expected-warning@-1 and so on... which is fine.

clang/test/SemaCXX/constant-expression-cxx14.cpp
3

17?

clang/test/SemaCXX/conversion-function.cpp
5

17?

clang/test/SemaCXX/coroutine-rvo.cpp
1

Surely this should be tested in 20 (and 2b), since coroutines are a C++20 feature.

clang/test/SemaCXX/deduced-return-type-cxx14.cpp
8

17?

clang/test/SemaCXX/return-stack-addr.cpp
3

14, 17?

mizvekov added inline comments.Mar 23 2021, 5:02 PM
clang/test/CXX/dcl.dcl/dcl.spec/dcl.type/dcl.spec.auto/p7-cxx14.cpp
3
  1. I skipped on purpose, felt like may have been adding more than is necessary, but I can add it no problem.
  2. It sucks I know, but not as far as I know. Something has to be implemented in lit for that to happen.
clang/test/CXX/drs/dr3xx.cpp
6

err, yes, I was not thinking very clearly :P

444

heh, no problem.

clang/test/CXX/temp/temp.decls/temp.mem/p5.cpp
8

Again, just avoiding the extra workload there, but I can add it.

clang/test/SemaCXX/constant-expression-cxx11.cpp
1770

Err the doubled one was a typo, but I was just keeping the local style.

clang/test/SemaCXX/coroutine-rvo.cpp
1

Yeah but remember, the coroutine change was unconditional, and I don't think this test is picking up any differences for the other modes.

mizvekov added inline comments.Mar 23 2021, 5:09 PM
mizvekov updated this revision to Diff 332832.Mar 23 2021, 5:18 PM

Changes per Arthur.

mizvekov updated this revision to Diff 332844.Mar 23 2021, 6:41 PM

Normalize test tag names on clang/test/SemaCXX/P1155.cpp

mizvekov marked 11 inline comments as done.Mar 24 2021, 3:36 PM
mizvekov updated this revision to Diff 335044.Apr 2 2021, 4:22 PM

Includes some new NRVO tests which will be used in D99696.

mizvekov retitled this revision from [clang] cxx tests: cleanup, update and add some new ones to [clang] tests: cleanup, update and add some new ones.Apr 2 2021, 4:56 PM

LGTM (just a bunch of style comments from me), but I think you'll have to get someone else's attention if you're expecting to get signoff from someone to land this.

I also think it would make sense for you to land the absolutely trivial trailing-whitespace diffs right now without review, and then rebase this PR on top, so that those trivial whitespace diffs don't show up as distractions in this review. (In case it's useful to anyone reading: I use detab -R to strip trailing spaces and convert hard tabs.)

clang/test/SemaCXX/constant-expression-cxx11.cpp
1907–1911

I guess you're keeping the existing style here, but the existing style is not great.

Incidentally, I don't understand why Clang's error message is so dramatically different between C++11 and C++14 modes — in C++14 through 20 we get "static_assert failed" but in C++11 it wants specifically to talk about that virtual function call.
https://godbolt.org/z/1e8nqTbsb

clang/test/SemaCXX/constant-expression-cxx14.cpp
230

Many of the existing snippets are IMHO too brief, but this new snippet was IMHO too verbose.

292–298

Here's an example of "existing too brief" plus "new too verbose." The most important snippet of the diagnostic actually doesn't change between 14 and 20!

clang/test/SemaCXX/conversion-function.cpp
146

Oh this is a fun one. I almost want to ask for a code comment here, but I suppose anyone who breaks this test should be prepared to do the archeology anyway. :)

Prior to C++20 this didn't consider using AutoPtr(AutoPtrRef) to construct the return value, and so the first overload resolution (as rvalue) failed to find any candidates, and the second overload resolution (as lvalue) found the private ctor. Then in C++20 and later, we do consider AutoPtr(AutoPtrRef) during the first overload resolution, and it works fine, so the second overload resolution never happens.

161

When is this condition true?
(Honestly I'm surprised that the expected-warning parser understands #if in the first place.)

clang/test/SemaCXX/coroutine-rvo.cpp
41–46

This change makes is_trivial_v<MoveOnly> true, where before it was false.
I don't know whether this matters to the coroutine machinery. Ideally, @aaronpuchert would weigh in; I don't know, and am just raising the issue because it struck my attention.

(If it does matter, then maybe we even want to add test coverage for both the move-only-trivial case and the move-only-nontrivial case.)

145

Nit: return_template_task or generic_task would look less confusingly like template return_task... at first glance. :)

clang/test/SemaCXX/coroutines.cpp
1127

Nit: Here and below, I'd be briefer.

mizvekov added a comment.EditedApr 2 2021, 5:42 PM

@Quuxplusone Thank you for the review. All your points are good ones and will be addressed in time!

The new NRVO propagation test clang/test/CodeGen/nrvo-tracking.cpp is also breaking on the buildbot, but it works on my machine.
Will also take a look at that one.

clang/test/SemaCXX/conversion-function.cpp
161

Too bad you can't also define macros for these expected declarations :)

mizvekov updated this revision to Diff 335055.Apr 2 2021, 6:27 PM

Just rebased, removing trailing whitespace noise.

Other review points to be addressed later.

mizvekov added inline comments.Apr 3 2021, 5:32 PM
clang/test/SemaCXX/conversion-function.cpp
161

From my reading of the code, only when compiling this source as not C++ (__cplusplus undefined).

And you made me realize I made a mistake there.

So remember this is called from -cc1, and when you don't pass "-std=c++" option to it, it looks like it does not actually set __cplusplus.

So yeah I unintentionally removed some test coverage there, which I will restore, thanks for catching this!

I will also look for if I have made any similar mistakes in other parts of this patch.

clang/test/SemaCXX/coroutine-rvo.cpp
41–46

So these are the changes I picked from D68845.

You were a reviewer, and you did not raise an objection there :)

Though that is perfectly fine, that was a couple of years back and our perceptions just change I guess.

Though once I have P2266 implementation rebased on top of this, I can see if this change still makes sense in our context, but I'd also like for @aaronpuchert to weight on this.

145

Like previous, you did not raise an objection on the original D68845 :)

But I agree with this change. But I'd like for @aaronpuchert to have a final say on this since these are his tests that I am merely taking, just so they do not sit there unused.

mizvekov updated this revision to Diff 335121.Apr 3 2021, 6:35 PM
  • Addresses most of Arthur's points.
  • Re-adds test coverage for conversion-function.cpp and temp.mem/p5.cpp with no C++ version specified.
  • Adds a couple more tests on nrvo-tracking for blocks returning references.
mizvekov marked 5 inline comments as done.Apr 3 2021, 6:37 PM
mizvekov added inline comments.Apr 6 2021, 11:07 AM
clang/test/SemaCXX/coroutine-rvo.cpp
41–46

I think I see why this change was made. Both in @aaronpuchert 's patch and in the P2266 implementation, we stop creating that (incorrect) temporary when passing value between co_return expression and return_value. A non-trivial value is a better test for the temporary, but it does not add anything to the case with no temporary.

So what I am going to do, is to move just this change to the P2266 implementation patch.

145

I will go ahead and make this change anyway, since he might be unavailable. We can always revert it if he comes back and raises an objection, I guess :)

mizvekov updated this revision to Diff 335599.Apr 6 2021, 11:08 AM
  • Finally addresses all of Arthur's review requests.
mizvekov marked 2 inline comments as done.Apr 6 2021, 11:14 AM

There's unlikely to be any further substantive comments from me, and this basically LGTM (or, in the places it looks bad, it's just the pre-existing awfulness of how the Clang test suite is organized).
I think you need to either get someone's attention to approve this, or approve it yourself. :)

clang/test/SemaCXX/constant-expression-cxx11.cpp
393–406

I wonder if you ought to just leave this test alone. It's got cxx11 in the name of the test, and it does a lot of things that are deprecated in 20/2b which you're having to work around.
No strong opinion, but mainly because I haven't looked closely.
If your upcoming NRVO patches actually have some effect on constexpr evaluation, then perhaps it's time to add a separate constant-expression-cxx20.cpp. Or, split out the NRVO-specific parts of this one into a smaller simpler constexpr-copy-elision.cpp which runs in all modes, but leave the bulk of this file alone and C++11-only. WDYT?

Ooh, same comment about constant-expression-cxx(1y->14).cpp. I do see that as evidence in favor of adding a constant-expression-cxx20.cpp.

thakis accepted this revision.Apr 6 2021, 12:08 PM
thakis added a subscriber: thakis.

This looks fine to me. Maybe wait for a day or two to see if richardsmith wants to say anything, else this is good to go.

This revision is now accepted and ready to land.Apr 6 2021, 12:08 PM
This revision was automatically updated to reflect the committed changes.