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

silence clang's -Wcomma #2543

Merged
merged 1 commit into from
Nov 14, 2022
Merged

Conversation

timblechmann
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Oct 6, 2022

Codecov Report

Merging #2543 (289d254) into devel (728de35) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##            devel    #2543      +/-   ##
==========================================
- Coverage   91.58%   91.57%   -0.01%     
==========================================
  Files         183      183              
  Lines        7564     7564              
==========================================
- Hits         6927     6926       -1     
- Misses        637      638       +1     

@horenmar
Copy link
Member

horenmar commented Oct 8, 2022

Can you provide an example of when this warning triggers?

@timblechmann
Copy link
Contributor Author

timblechmann commented Oct 9, 2022

my code looks like this:

#include <catch2/catch_template_test_macros.hpp>

namespace a::b::c {

struct foo {};

using tests = std::tuple< foo >;

TEMPLATE_LIST_TEST_CASE( "test", "", tests )
{}

} // namespace a::b::c

(it seems to be triggered by putting the TEMPLATE_LIST_TEST_CASE macro into the namespace)

@horenmar
Copy link
Member

I tried to quickly reproduce this on godbolt, but can't.

Given that this PR would silence -Wcomma for the entire test case code, including user's code, I do not want to merge it without a reproducer to see what the issue actually is, and that it is part of our code (as opposed to user code, whose warnings we should not touch.).

@timblechmann
Copy link
Contributor Author

i've tried to reproduce it again. it seems to be triggered by using a clang icecc toolchain with local proprocessing. so probably the namespace was a red herring.

fwiw, the offending comma is this one

Catch::NameAndTags{ "test" " - " + std::string("tests") + " - " + std::to_string(index), "" } ), index++)... }; } }
                                                                                               ^

@horenmar
Copy link
Member

Hi, sorry for not getting to this earlier. I thought the changes would silence -Wcomma in the test code as well, so I put it in my backlog to return to later.

Today I did, and turns out that suppression is already limited to the registration only.

@horenmar horenmar merged commit e932bcf into catchorg:devel Nov 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants