On Tue, 30 Jul 2024 at 14:08, Jonathan Wakely <jwakely....@gmail.com> wrote:
>
> On Tue, 30 Jul 2024 at 08:31, Giuseppe D'Angelo
> <giuseppe.dang...@kdab.com> wrote:
> >
> > Hello!
> >
> > The attached patch implements adds support for P2591R5 in libstdc++
> > (concatenation of strings and string_views, approved in Tokyo for C++26).
>
> Thanks for this patch as well. This was on my TODO list so I'll be
> happy to not have to do it myself now!
>
> I won't repeat my questions for your is_virtual_base_of patch,
> regarding the legal prerequisites, but they apply here too. My
> comments about copyright and licence headers in the tests apply here
> too.
>
> +#if __cplusplus > 202302L
> + // const string & + string_view
>
> Please test the feature test macro, not the C++ standard version, so e.g.
>
> #if __glibcxx_string_view >= 202403L
>
> This relates the #if block directly to the feature test macro, so that
> if we decide to support the feature in C++23 as an extension, we only
> need to change version.def and not basic_string.h
Also the use of dg-options and { target c++NN } selectors in the tests
is wrong. Please read
https://gcc.gnu.org/onlinedocs/libstdc++/manual/test.html#test.new_tests
+// { dg-do compile { target c++17 } }
+// { dg-error "no match for" "P2591" { target c++17 } 25 }
+// { dg-options "-std=gnu++2c" }
+// { dg-do compile { target c++17 } }
+// { dg-error "no match for" "P2591" { target c++26 } 25 }
Why are there two tests for this?
The test will fail for any mode C++17 or later, so you only need one
test, and so no need for a separate op_plus_fspath_impl.h header. Just
write one test with:
+// { dg-do compile { target c++17 } }
+// { dg-error "no match for" "P2591" { target c++17 } 25 }
The "P2591" comment isn't very clear, the paper only mentions fs::path once:
"Added the requested tests (involving std::filesystem::path and a test
producing ambiguities) to the working prototype."
This doesn't really explain what the test is for, could you add a
comment to the new test saying that the new operators added by P2591
are expected to not support concatenation with paths. And move the
content of op_plus_fspath_impl.h into op_plus_fspath_cpp17_fail.cc and
get rid of op_plus_fspath_cpp2c_fail.cc. With that change, you'll also
be able to put the dg-error on the right line where the error actually
happens.
+#if !defined(__cpp_lib_string_view) || __cpp_lib_string_view < 202403L
+#error
+#endif
The #error should say what's wrong, otherwise the logs would just show:
FAIL: op_plus_string_view.cc -std=gnu++17
#error
which is not very helpful.
+// { dg-options "-std=gnu++26" }
+// { dg-do run { target c++20 } }
This is entirely bogus, it says the test should run for C++20 and
later, but then forces it to run for only 26.
This should be just:
// { dg-do run { target c++26 } }
The testsuite will add the right -std option, so don't force it with dg-options.
+// { dg-options "-std=gnu++2c" }
+// { dg-do compile { target c++17 } }
+// { dg-error "ambiguous" "P2591" { target c++26 } 73 }
Don't force -std with dg-options, just use the right target selector.
Please add a comment explaining what is being tested here, "P2591"
doesn't tell me.
Please combine op_plus_string_view_compat_ok.cc and
op_plus_string_view_compat_fail.cc into one file, and get rid of the
impl header.
I think it should be a single file with something like:
// { dg-do run { target { c++17 && c++23_down } } }
// { dg-do compile { target c++26 } }
// This should be ambiguous in C++26 due to new operator+ overloads.
// { dg-error "ambiguous" "P2591" { target c++26 } 73 }
This say to make it a "run" test for C++17-23 and an xfail compile
test for C++23 and later.
Although I don't see any reason to make it a { dg-do run } test for
C++17, all that's testing is that your my_string_view concatenation
operators work correctly, which is irrelevant to libstdc++ code.
So it could be just
// { dg-do compile { target c++17 } }
// This should be ambiguous in C++26 due to new operator+ overloads.
// { dg-error "ambiguous" "P2591" { target c++26 } 73 }
i.e. compile for all modes C++17 and later, but expect an error for
C++26 and later.
Thanks for providing both runtime and constexpr tests, that's great.
