Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tests trigger new -Wreserved-identifier in clang 13+ #1388

Closed
jkelling opened this issue Aug 10, 2021 · 16 comments · Fixed by #1485
Closed

Tests trigger new -Wreserved-identifier in clang 13+ #1388

jkelling opened this issue Aug 10, 2021 · 16 comments · Fixed by #1485

Comments

@jkelling
Copy link
Member

jkelling commented Aug 10, 2021

A new warning was added in clang main which is triggered by the tests:

error: identifier '____C_A_T_C_H____T_E_M_P_L_A_T_E____T_E_S_T____1' is reserved because it starts with '__' [-Werror,-Wreserved-identifier]

-Wno-reserved-identifier may need to be added to CXX_FLAGS for clang 13 and 14 (maybe also clang 13, have to check.)

@sbastrakov
Copy link
Member

I think this is a defect of catch2, so we should report there?

@j-stephan
Copy link
Member

They are aware: catchorg/Catch2#578

@j-stephan
Copy link
Member

Marking this as Wontfix for now since this is third party issue. Let's see what the Catch2 team will do about this first.

@sbastrakov
Copy link
Member

Unrelated to this issue, but should we add ourselves (alpaka, cupla, PIConGPU) to catch2 user page?

@j-stephan
Copy link
Member

Sure, more visibility is always welcome :-)

@SimeonEhrig
Copy link
Member

Unrelated to this issue, but should we add ourselves (alpaka, cupla, PIConGPU) to catch2 user page?

I would suggest we name PIConGPU and the alpaka-group. vikunja use it also and I think lamini will it also do.

@sbastrakov
Copy link
Member

I agree, an umbrella "alpaka group" is reasonable.

@BenjaminW3
Copy link
Member

We definitely should not declare this as wontfix because it is a compilation error of our tests (due to -Werror) with clang 14.
We do not yet officially support it, but will do so in the future.
We probably should add an exception for this warning and not make it an error via -Wno-error=reserved-identifier.

@j-stephan
Copy link
Member

I wonder why this warning is visible anyway. We already include the Catch2 header(s) as SYSTEM in CMake which should silence any warning emitted from them.

We definitely should not declare this as wontfix because it is a compilation error of our tests (due to -Werror) with clang 14.

Note the for now in the sentence ;-) If we still see this error when clang 14 is available (we don't even have clang 13 yet) we can think about manually silencing the warning. Until then I'd like to wait for the Catch2 developers to fix this:

The warning is triggered because Catch2 is in violation of the C++ standard. Using -Werror is a sensible thing to do for test cases, a test library should reflect this.

@jkelling jkelling changed the title Tests trigger new -Wreserved-identifier in clang 14 (main) Tests trigger new -Wreserved-identifier in clang 13+ Aug 12, 2021
@jkelling
Copy link
Member Author

I checked, updated the description before, and now updated the title: This flag will already be present in clang 13 🚀 , so, quite soon, 💥 .

@jkelling
Copy link
Member Author

jkelling commented Aug 12, 2021

I wonder why this warning is visible anyway. We already include the Catch2 header(s) as SYSTEM in CMake which should silence any warning emitted from them.

This is probably because the identifier '____C_A_T_C_H____T_E_M_P_L_A_T_E____T_E_S_T____1' is generated by a macro invoked in our code.

@j-stephan
Copy link
Member

According to the issue I linked above this will be resolved in Catch2's next major release (v3). It remains to be seen if this will be backported to v2 which we currently use,

@sbastrakov
Copy link
Member

However catch3 will have a different interface from catch2. So we would really like a backport, since I think we are happy with catch2 generally (and i think our testing demands are not that specific) and then changing to another interface may be extra work for little gain.

@bernhardmgruber
Copy link
Member

I just ran into the same issue with clang 14:

/mnt/c/dev/alpaka/test/integ/mandelbrot/src/mandelbrot.cpp:252:1: error: identifier '____C_A_T_C_H____T_E_M_P_L_A_T_E____T_E_S_T____F_U_N_C____0' is reserved because it starts with '__' [-Werror,-Wreserved-identifier]
TEMPLATE_LIST_TEST_CASE("mandelbrot", "[mandelbrot]", TestAccs)
^
/mnt/c/dev/alpaka/test/catch_main/../../thirdParty/catch2/include/catch2/catch.hpp:17633:40: note: expanded from macro 'TEMPLATE_LIST_TEST_CASE'
#define TEMPLATE_LIST_TEST_CASE( ... ) INTERNAL_CATCH_TEMPLATE_LIST_TEST_CASE(__VA_ARGS__)
                                       ^
/mnt/c/dev/alpaka/test/catch_main/../../thirdParty/catch2/include/catch2/catch.hpp:1204:130: note: expanded from macro 'INTERNAL_CATCH_TEMPLATE_LIST_TEST_CASE'
        INTERNAL_CATCH_TEMPLATE_LIST_TEST_CASE_2( INTERNAL_CATCH_UNIQUE_NAME( ____C_A_T_C_H____T_E_M_P_L_A_T_E____T_E_S_T____ ), INTERNAL_CATCH_UNIQUE_NAME( ____C_A_T_C_H____T_E_M_P_L_A_T_E____T_E_S_T____F_U_N_C____ ), Name, Tags, TmplList )
                                                                                                                                 ^
/mnt/c/dev/alpaka/test/catch_main/../../thirdParty/catch2/include/catch2/catch.hpp:466:46: note: expanded from macro 'INTERNAL_CATCH_UNIQUE_NAME'
#  define INTERNAL_CATCH_UNIQUE_NAME( name ) INTERNAL_CATCH_UNIQUE_NAME_LINE( name, __COUNTER__ )
                                             ^
/mnt/c/dev/alpaka/test/catch_main/../../thirdParty/catch2/include/catch2/catch.hpp:464:55: note: expanded from macro 'INTERNAL_CATCH_UNIQUE_NAME_LINE'
#define INTERNAL_CATCH_UNIQUE_NAME_LINE( name, line ) INTERNAL_CATCH_UNIQUE_NAME_LINE2( name, line )
                                                      ^
/mnt/c/dev/alpaka/test/catch_main/../../thirdParty/catch2/include/catch2/catch.hpp:463:56: note: expanded from macro 'INTERNAL_CATCH_UNIQUE_NAME_LINE2'
#define INTERNAL_CATCH_UNIQUE_NAME_LINE2( name, line ) name##line
                                                       ^
<scratch space>:26:1: note: expanded from here
____C_A_T_C_H____T_E_M_P_L_A_T_E____T_E_S_T____F_U_N_C____0

@j-stephan
Copy link
Member

Judging by the catch2 issue linked above a fix will appear in the next Catch2 release (whenever that is).

@j-stephan j-stephan removed this from To do in Release 0.8 Nov 10, 2021
@j-stephan j-stephan added this to To do in Release 0.9 via automation Nov 10, 2021
@j-stephan
Copy link
Member

Since it is unlikely that the next Catch2 version is released this week I move this into the alpaka 0.9 project. We can then maybe backport this to 0.8.1.

@j-stephan j-stephan self-assigned this Nov 10, 2021
This was referenced Dec 10, 2021
@j-stephan j-stephan linked a pull request Dec 16, 2021 that will close this issue
2 tasks
bernhardmgruber added a commit to bernhardmgruber/alpaka that referenced this issue Dec 17, 2021
Release 0.9 automation moved this from To do to Done Dec 17, 2021
psychocoderHPC pushed a commit that referenced this issue Dec 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

6 participants