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

#pragma clang diagnostic ignored "-Wpadded" leaves scope #871

Closed
tomvierjahn opened this issue Mar 28, 2017 · 7 comments
Closed

#pragma clang diagnostic ignored "-Wpadded" leaves scope #871

tomvierjahn opened this issue Mar 28, 2017 · 7 comments

Comments

@tomvierjahn
Copy link

Description

I am using clang and -Wpadded.

#include "foo.hpp"
#include "catch/catch.hpp

and clang complains about padding in class Foo, as desired.

Whereas

#include "catch/catch.hpp
#include "foo.hpp"

leads to no warnings.

It seems that some #pragma clang diagnostic ignored "-Wpadded" are not enclosed by push/pop in the single header version.

Steps to reproduce

see above

Extra information

  • Catch version: v1.6.0
  • Operating System: MacOs X 10.12.3
  • Compiler+version: Apple LLVM version 8.0.0 (clang-800.0.42.1)
@horenmar
Copy link
Member

Can you also check with current Catch? I suspect it will still be there, but ...

Also it might be intentional -- Catch generates some code in the test files, so we have to leak suppressions to the test file as well -- but in this case I don't think it is and will have to check.

@tomvierjahn
Copy link
Author

tomvierjahn commented Mar 29, 2017

It still does.

  • Catch version: v2.0.0-develop.3 (git hash afc0b31)

@horenmar
Copy link
Member

@tomvierjahn That is not the current version. We probably should have deleted that branch when we went back to fixing Catch Classic, but it was left around in case we wanted to backport some changes.

Using Catch v1.8.2 and compiling this file

struct foobar {
    bool b;
    int i;
};

#define CATCH_CONFIG_MAIN
#include "catch.hpp"

TEST_CASE("aa") {
    REQUIRE(1 == 2);
}

void foo() {
    foobar fb;
}

Triggers -Wpadded, so I am going to close this issue.

xarn@DESKTOP-A79J11O:~/scratch$ clang++-3.8 test.cpp -Weverything -Wno-missing-prototypes -Wno-exit-time-destructors
test.cpp:5:9: warning: padding struct 'foobar' with 3 bytes to align 'i' [-Wpadded]
    int i;
        ^
1 warning generated.

@onqtam
Copy link

onqtam commented Mar 29, 2017

I think the problem is when the struct is defined after the catch header and not before, but the issue seems fixed - online compiler - triggers the warning as desired

@horenmar
Copy link
Member

@onqtam My brief testing showed that Clang cares about instantiation of the structs, not definition, when issuing -Wpadded.

@tomvierjahn
Copy link
Author

Unfortunately, it is not fixed yet.
Furthermore, it shows "strange" behaviour …

Setup

  • Catch version (from catch.hpp), git hash 0c015aa:
*  Catch v1.8.2
*  Generated: 2017-03-13 21:18:33.619572
  • clang: Apple LLVM version 8.1.0 (clang-802.0.38)
  • warning levels:
-Werror
-Weverything
-pedantic
-pedantic-errors
-Wextra-tokens
-Wambiguous-member-template
-Wbind-to-temporary-copy
-Wno-c++98-compat

Case 1:

  • single file only: test_foo.cpp
  • shows expected behaviour

variant A

test_foo.cpp:

#define CATCH_CONFIG_MAIN
#include "catch/catch.hpp"

class A {
public:
  float f;
  bool i;
};

#pragma clang diagnostic ignored "-Wexit-time-destructors"
TEST_CASE("padding", "[padding]") {
  A a;
  a.i = true;
  CHECK(a.i);
}

warning:

padding size of 'A' with 3 bytes to alignment boundary [-Werror,-Wpadded]

variant B

test_foo.cpp:

#define CATCH_CONFIG_MAIN

class A {
public:
  float f;
  bool i;
};

#include "catch/catch.hpp"

#pragma clang diagnostic ignored "-Wexit-time-destructors"
TEST_CASE("padding", "[padding]") {
  A a;
  a.i = true;
  CHECK(a.i);
}

warning:

padding size of 'A' with 3 bytes to alignment boundary [-Werror,-Wpadded]

variant C

test_foo.cpp:

class A {
public:
  float f;
  bool i;
};

#define CATCH_CONFIG_MAIN
#include "catch/catch.hpp"

#pragma clang diagnostic ignored "-Wexit-time-destructors"
TEST_CASE("padding", "[padding]") {
  A a;
  a.i = true;
  CHECK(a.i);
}

warning:

padding size of 'A' with 3 bytes to alignment boundary [-Werror,-Wpadded]

Case 2

  • separate files test.cpp and test_foo.cpp
  • behaviour depends on the position of `#include "catch/catch.hpp"

test.cpp

#define CATCH_CONFIG_MAIN
#include "catch/catch.hpp"

variant A:

test_foo.cpp:

#include "catch/catch.hpp"

class A {
public:
  float f;
  bool i;
};

#pragma clang diagnostic ignored "-Wexit-time-destructors"
TEST_CASE("padding", "[padding]") {
  A a;
  a.i = true;
  CHECK(a.i);
}

no warning

variant B:

test_foo.cpp:

class A {
public:
  float f;
  bool i;
};

#include "catch/catch.hpp"

#pragma clang diagnostic ignored "-Wexit-time-destructors"
TEST_CASE("padding", "[padding]") {
  A a;
  a.i = true;
  CHECK(a.i);
}

warning:

padding size of 'A' with 3 bytes to alignment boundary [-Werror,-Wpadded]

@horenmar horenmar reopened this Apr 3, 2017
@horenmar
Copy link
Member

horenmar commented Jun 2, 2017

I finally got back to this and it should now be fixed in master (not in the single include yet).

It turned out that this was by interplay between superfluous include, and our single header generator, making it so that a pragma diagnostic pop that was supposed to be at the end of the single include header, was instead inside code that is guarded by CATCH_IMPL define. This meant that in test files, there was 1 more pragma diagnostic push than there were pops.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants