This is an archive of the discontinued LLVM Phabricator instance.

[libc++] [test] [NFC] Flatten the directory structure a bit
ClosedPublic

Authored by Quuxplusone on Dec 22 2021, 5:28 PM.

Details

Summary

These test files for [ranges.access] are all pretty similar, and hiding them in one-file-per-directory doesn't accomplish anything (except perhaps hiding the fact that we don't have very many tests for cbegin and cend). Pull them all up to the same level, so we can see that they're all present and accounted for.

Diff Detail

Event Timeline

Quuxplusone requested review of this revision.Dec 22 2021, 5:28 PM
Quuxplusone created this revision.
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptDec 22 2021, 5:28 PM
var-const accepted this revision.Dec 22 2021, 5:47 PM
philnik accepted this revision.Dec 23 2021, 3:48 AM

I don't necessarily mind moving them, but after this changes searching for a path containing range.prim turns up empty.
I would prefer names that contain the section name, for example
libcxx/test/std/ranges/range.access/range.access.begin.pass.cpp
libcxx/test/std/ranges/range.access/range.prim.data.pass.cpp

Mordante accepted this revision as: Mordante.Dec 23 2021, 11:34 AM

As discussed on Discord I'm oke with this, but let's discuss it with @ldionne after the holidays.

Quuxplusone accepted this revision.Dec 23 2021, 11:36 AM

Sounds like a plan. :)

This revision is now accepted and ready to land.Dec 23 2021, 11:36 AM

Can someone summarize the discussion? Previously the test paths were mirroring section names in the standard. I'd like to understand what the proposed naming scheme is.

Can someone summarize the discussion? Previously the test paths were mirroring section names in the standard. I'd like to understand what the proposed naming scheme is.

The new test paths mirror the section names in the standard: everything that is specified under http://eel.is/c++draft/range.access is now located under libcxx/test/*/ranges/range.access/.

$ ls ../libcxx/test/*/ranges/range.access/
../libcxx/test/libcxx/ranges/range.access/:
begin.incomplete_type.sh.cpp	end.incomplete_type.pass.cpp

../libcxx/test/std/ranges/range.access/:
begin.pass.cpp	data.pass.cpp	empty.pass.cpp	end.pass.cpp	size.pass.cpp	ssize.pass.cpp

The old test paths were kind of wonky, because they included subdirectory components that didn't mirror the standard's stablenames. E.g. data, size, and ssize were previously tested under the subdirectory libcxx/test/std/ranges/range.access/range.prim/, but there is no https://eel.is/c++draft/range.prim in the standard.

@Mordante suggested that it might be better to name the test files after the corresponding stablenames, instead of the corresponding library identifiers:

begin.pass.cpp => range.access.begin.pass.cpp
end.pass.cpp => range.access.end.pass.cpp
data.pass.cpp => range.prim.data.pass.cpp
empty.pass.cpp => range.prim.empty.pass.cpp
size.pass.cpp => range.prim.size.pass.cpp
ssize.pass.cpp => range.prim.ssize.pass.cpp

I'm only like 80% opposed to that idea. :) The current names are shorter/easier to decode (e.g. empty is tested in empty.pass.cpp, not something with the word empty buried in the middle of the name). The short names are also slightly better at hiding the contradiction that cbegin is tested in the same file as begin: it seems pretty defensible that begin.pass.cpp tests cbegin too, whereas if we call it range.access.begin.pass.cpp then we have to explain the absence of range.access.cbegin.pass.cpp. (This applies to end/cend, rbegin/crbegin, rend/crend, and data/cdata. It should probably also apply to size/ssize, but we haven't gotten around to rewriting that pair of test files into a single file yet. I suspect a lot of test coverage is missing for ssize.)

Can someone summarize the discussion? Previously the test paths were mirroring section names in the standard. I'd like to understand what the proposed naming scheme is.

The new test paths mirror the section names in the standard: everything that is specified under http://eel.is/c++draft/range.access is now located under libcxx/test/*/ranges/range.access/.
[...]
The old test paths were kind of wonky, because they included subdirectory components that didn't mirror the standard's stablenames. E.g. data, size, and ssize were previously tested under the subdirectory libcxx/test/std/ranges/range.access/range.prim/, but there is no https://eel.is/c++draft/range.prim in the standard.

Okay, in that case my affirmation above was wrong. Thanks for clarifying.

@Mordante suggested that it might be better to name the test files after the corresponding stablenames, instead of the corresponding library identifiers:

begin.pass.cpp => range.access.begin.pass.cpp
end.pass.cpp => range.access.end.pass.cpp
data.pass.cpp => range.prim.data.pass.cpp
empty.pass.cpp => range.prim.empty.pass.cpp
size.pass.cpp => range.prim.size.pass.cpp
ssize.pass.cpp => range.prim.ssize.pass.cpp

I'm not a huge fan of this either, for the same reasons you stated. It would be more consistent if we did this throughout, though, but it would be a pretty big change (a simple change but it would touch a lot of files). Today, we basically use section names except for the leaf name where we use "whatever makes sense", or at least that's how I would codify what I think we're doing.

Thanks for your input.

@Mordante suggested that it might be better to name the test files after the corresponding stablenames, instead of the corresponding library identifiers:

begin.pass.cpp => range.access.begin.pass.cpp
end.pass.cpp => range.access.end.pass.cpp
data.pass.cpp => range.prim.data.pass.cpp
empty.pass.cpp => range.prim.empty.pass.cpp
size.pass.cpp => range.prim.size.pass.cpp
ssize.pass.cpp => range.prim.ssize.pass.cpp

I'm not a huge fan of this either, for the same reasons you stated. It would be more consistent if we did this throughout, though, but it would be a pretty big change (a simple change but it would touch a lot of files). Today, we basically use section names except for the leaf name where we use "whatever makes sense", or at least that's how I would codify what I think we're doing.

I basically wanted that style since it keeps the omitted section names, including the odd "prim". But I've no objection against the shorter names.

libcxx/test/std/ranges/range.access/empty.pass.cpp