This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Adds a make_string test helper function.
ClosedPublic

Authored by Mordante on Dec 16 2020, 11:12 AM.

Details

Summary

These function makes it easier to write generic unit tests for the
format header. It solves the issue where it's not possible to use

`templated_prefix"foo"`

where templated_prefix resolves to: nothing, L, u8, u,
or U. The templated_prefix would be more faster during execution.

Diff Detail

Event Timeline

Mordante requested review of this revision.Dec 16 2020, 11:12 AM
Mordante created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptDec 16 2020, 11:12 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante updated this revision to Diff 312276.Dec 16 2020, 11:56 AM

Disable unit test when localization is disabled.

How about something more like:

make_string<char>(
                  " !\"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMN"
                          "OPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~")
      ==          " !\"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMN"
                          "OPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~");

instead?

libcxx/test/support/test.support/make_string_header.pass.cpp
24

the indentation here makes it hard to see that the literals are supposed to be identical.

Mordante marked an inline comment as done.Dec 17 2020, 7:52 AM

Thanks for the review!

libcxx/test/support/test.support/make_string_header.pass.cpp
24

I'll have a look at manually formatting the code.

Mordante updated this revision to Diff 312506.Dec 17 2020, 8:04 AM
Mordante marked an inline comment as done.

Improve the formatting as requested by @mclow.lists.
Disabled the automatic formatting to make sure the formatting won't be updated
automatically.

mclow.lists added inline comments.Dec 17 2020, 10:01 AM
libcxx/test/support/test.support/make_string_header.pass.cpp
24

Much easier to read; thanks.

curdeius accepted this revision.Jan 26 2021, 7:39 AM
curdeius added subscribers: mstorsjo, curdeius.

LGTM. Just 2 nits.
Have you looked at what @mstorsjo did in his series of patches for porting filesystem to Windows? I have a vague idea that there were some converting utilities similar to this one.

libcxx/test/support/make_string.h
24

Nit: "there are no" instead of "are no".

31

Nit: convertor? Why not converter?

LGTM. Just 2 nits.
Have you looked at what @mstorsjo did in his series of patches for porting filesystem to Windows? I have a vague idea that there were some converting utilities similar to this one.

Thanks for the review. I've didn't have a close look at @mstorsjo's changes. @mstorsjo do you have code in review with similar functionality?

libcxx/test/support/make_string.h
24

Will fix.

31

As far as I know they are synonyms, but I'm no native speaker. But it seems converter is more common, so will adjust the code.

Mordante updated this revision to Diff 319610.Jan 27 2021, 10:08 AM

Addresses review comments.
Rebased.

Mordante marked 3 inline comments as done.Jan 27 2021, 10:08 AM

LGTM. Just 2 nits.
Have you looked at what @mstorsjo did in his series of patches for porting filesystem to Windows? I have a vague idea that there were some converting utilities similar to this one.

Thanks for the review. I've didn't have a close look at @mstorsjo's changes. @mstorsjo do you have code in review with similar functionality?

Not entirely sure what that'd refer to - maybe there's some similarity to https://reviews.llvm.org/D90222#change-3lQgyuTryhdf - with the MultiStringType class that I modified for similar cases (with some char types being optionally available).

ldionne requested changes to this revision.Jan 28 2021, 8:17 AM
ldionne added inline comments.
libcxx/test/support/make_string.h
26

That is not going to work when the test suite is run against non-libc++. Can we replace by something that doesn't depend on libc++?

This revision now requires changes to proceed.Jan 28 2021, 8:17 AM

Not entirely sure what that'd refer to - maybe there's some similarity to https://reviews.llvm.org/D90222#change-3lQgyuTryhdf - with the MultiStringType class that I modified for similar cases (with some char types being optionally available).

Yes it seems MultiStringType does something similar. I've to see how well it fits in my unit tests. Thanks for the hint!

libcxx/test/support/make_string.h
26

I had a look and test/std/depr/depr.c.headers/stdio_h.pass.cpp uses

#if defined(__GNUC__) || defined(__clang__)
#pragma GCC diagnostic ignored "-Wformat-zero-length"
#pragma GCC diagnostic ignored "-Wdeprecated-declarations" // for tmpnam
#endif

But I'll first have a look at MultiStringType. If that suits my needs this revision seems obsolete.

Mordante updated this revision to Diff 320859.Feb 2 2021, 11:05 AM

Changed the implementation to use the macros from
libcxx/test/support/filesystem_test_helper.h.
The macros are moved and the original header now depends on the new helper.

Mordante marked an inline comment as done.
Mordante added inline comments.
libcxx/test/support/make_string.h
26

That is not going to work when the test suite is run against non-libc++. Can we replace by something that doesn't depend on libc++?

With the changed implementation this code is entirely removed.

ldionne accepted this revision.Feb 2 2021, 12:06 PM

Thanks!

This revision is now accepted and ready to land.Feb 2 2021, 12:06 PM
This revision was automatically updated to reflect the committed changes.