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.

Sign up for GitHub

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

Trying to suppress all clang-target-msvc test warning in CMAKE and other misc fixes #1151

Merged
merged 36 commits into from
May 11, 2019
Merged

Trying to suppress all clang-target-msvc test warning in CMAKE and other misc fixes #1151

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
c575d56
Fix conditional `char8_t` from `format.h` and fix `-Wunused-result` o…
denchat May 8, 2019
28588a9
Suppress `-Winconsistent-dllimport` when in clang-target-msvc
denchat May 8, 2019
abb3050
Suppress warning _CRT_SECURE_NO_WARNINGS in MSVC and -Wdeprecated-dec…
denchat May 8, 2019
63d6445
Turn on exceptions automatically in MSVC
denchat May 8, 2019
c8cbf57
Use 'using' to provide char8_t type if neccessary as Victor's suggestion
denchat May 9, 2019
d75c98b
Narrowing scope to clang-target-msvc for mirroring cl default /EHsc
denchat May 9, 2019
d7b83ff
Narrowing /EHsc to clang-target-msvc only
denchat May 9, 2019
073571a
missing that semicolon!
denchat May 9, 2019
eb420be
Receive FMT_MSVC_EH from user
denchat May 9, 2019
b17216a
Receive FMT_MSVC_EH from user
denchat May 9, 2019
e7e48c3
Fix excessing parenthesis
denchat May 9, 2019
c079096
Fix excessing parenthesis
denchat May 9, 2019
9d0de74
Fix AppleClang match
denchat May 9, 2019
6593a07
Fix AppleClang match
denchat May 9, 2019
bdc4929
Try workaround with WIN32
denchat May 9, 2019
28b559e
Try workaround with WIN32
denchat May 9, 2019
133af4a
Because TravisCI evaluates MSVC true in linux and apple
denchat May 9, 2019
39a140a
Because TravisCI evaluates MSVC true in linux and apple
denchat May 9, 2019
032ac28
Bring _VARIADIC_MAX=10 out of MSVC AND WIN32 context
denchat May 9, 2019
8d8f7b1
gmock PUBLIC
denchat May 9, 2019
473ccd0
Update CMakeLists.txt
denchat May 9, 2019
ade161d
Update CMakeLists.txt
denchat May 9, 2019
ff44d1a
Update CMakeLists.txt
denchat May 9, 2019
03d7600
Not sure about MSVC, test travis again
denchat May 9, 2019
b6db4b8
Turns out that I misunderstood MSVC
denchat May 9, 2019
9155a22
Add conditional defined FMT_MAYBE_UNUSED
denchat May 10, 2019
eaaa44c
Use [[maybe_unused]] to suppress -Wunused-variable
denchat May 10, 2019
02e472d
Delete selective adding /EHsc. Leave it to users.
denchat May 11, 2019
f1fd5ac
Rollback
denchat May 11, 2019
659567c
Because grisu_format does "import" when `sizeof(Double) == sizeof(uin…
denchat May 11, 2019
889f904
Rollback
denchat May 11, 2019
8b3227f
(void)X; suppresses -Wunused-variable
denchat May 11, 2019
6d0343f
bool grisu_format is not supposed to be imported/exported
denchat May 11, 2019
166b79d
grisu_format is not supposed to be imported/exported
denchat May 11, 2019
3b4928a
Rollback
denchat May 11, 2019
e422dad
Remove FMT_FUNC, mark FMT_API to export
denchat May 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,9 @@ endif ()
if (FMT_PEDANTIC)
target_compile_options(fmt PRIVATE ${PEDANTIC_COMPILE_FLAGS})
endif ()
if (MSVC)
target_compile_options(fmt PRIVATE /EHsc)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exceptions are enabled by default on MSVC. Why is this needed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would also force the library into a specific exception handling model, which would be undesirable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vitaut : My clang-target-msvc isn't defaulting /EHsc. Then I guess this should just trying to mirror /EHsc default in MSVC cl.exe. Perhaps...

if (MSVC)
should be changed into
if (MSVC AND CMAKE_CXX_COMPILER_ID MATCHES "Clang" )

@eliaskosunen Would you be okay with narrowing to clang-target-msvc for mirroring the default /EHsc above?

Should there be FMT_MSVC_EH option that explicitly add custom /EHxx?

Copy link
Contributor Author

@denchat denchat May 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, perhaps we can have FMT_MSVC_EH like this :

if (MSVC)
  if (FMT_MSVC_EH)
    set(FMT_CUSTOM_MSVC_EH ${FMT_MSVC_EH})
  else ()
    if (CMAKE_CXX_COMPILER_ID MATCHES "Clang")
      set(FMT_CUSTOM_MSVC_EH /EHsc)
    else ()
      set(FMT_CUSTOM_MSVC_EH)
    endif ()
  endif ()
  target_compile_options(fmt PRIVATE ${FMT_CUSTOM_MSVC_EH})
endif ()

What do you guys think?

Copy link
Contributor Author

@denchat denchat May 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why TravisCI evaluates MSVC true in apple platform and linux clang platform.
So I try to workaround with

if (MSVC AND WIN32)
  if (FMT_MSVC_EH)
    set(FMT_CUSTOM_MSVC_EH ${FMT_MSVC_EH})
  else ()
    if (CMAKE_CXX_COMPILER_ID MATCHES "Clang")
      set(FMT_CUSTOM_MSVC_EH /EHsc)
    else ()
      set(FMT_CUSTOM_MSVC_EH)
    endif ()
  endif ()
  target_compile_options(fmt PRIVATE ${FMT_CUSTOM_MSVC_EH})
endif ()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that it's because my excessing endif, not MSVC weirdness. Fixed it.

Copy link
Contributor

@vitaut vitaut May 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My clang-target-msvc isn't defaulting /EHsc.

So what is the default in clang-target-msvc? Are exceptions disabled by default or enabled with a different setting?

Copy link
Contributor Author

@denchat denchat May 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

C:\Users\User\AppData\Roaming\fmt>ninja -j 2
[1/47] Building CXX object CMakeFiles\fmt.dir\src\posix.cc.obj
FAILED: CMakeFiles/fmt.dir/src/posix.cc.obj
C:\PROGRA~1\LLVM\bin\clang-cl.exe  /nologo -TP -DFMT_EXPORT -Dfmt_EXPORTS -IC:\Users\User\AppData\Roaming\fmt-master\include /MD /O2 /Ob2 /DNDEBUG   -std:c++latest /showIncludes /FoCMakeFiles\fmt.dir\src\posix.cc.obj /FdCMakeFiles\fmt.dir\ -c C:\Users\User\AppData\Roaming\fmt-master\src\posix.cc
In file included from C:\Users\User\AppData\Roaming\fmt-master\src\posix.cc:13:
In file included from C:\Users\User\AppData\Roaming\fmt-master\include\fmt/posix.h:28:
C:\Users\User\AppData\Roaming\fmt-master\include\fmt/format.h(121,10): error: cannot use 'throw' with exceptions disabled
  if (b) throw x;
         ^

It seems exceptions are disabled by default. Well, you are right, after take a look at => user manual, I found that users can explicitly state the configurations through very generous means. However, clang-cl.exe alone basically does not enable exceptions by default.

Nevertheless, if you think this compiler case wouldn't be worth for the added complexity, I will be fine with dropping these CMAKE changes for /EHsc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed conditional adding /EHsc from CMAKELists.txt in both root and test.

endif ()

target_compile_features(fmt INTERFACE ${FMT_REQUIRED_FEATURES})

Expand Down
9 changes: 8 additions & 1 deletion test/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,16 @@ if (NOT SUPPORTS_VARIADIC_TEMPLATES)
target_compile_definitions(gmock PUBLIC GTEST_LANG_CXX11=0)
endif ()

# Workaround a bug in implementation of variadic templates in MSVC11.
if (MSVC)
# Workaround a bug in implementation of variadic templates in MSVC11.
target_compile_definitions(gmock PUBLIC _VARIADIC_MAX=10)
# Automatic enable exception
target_compile_options(gmock PUBLIC /EHsc)
# Disable MSVC warning of _CRT_INSECURE_DEPRECATE functions and POSIX functions
target_compile_definitions(gmock PUBLIC _CRT_SECURE_NO_WARNINGS)
if (CMAKE_CXX_COMPILER_ID MATCHES "Clang")
target_compile_options(gmock PUBLIC -Wno-deprecated-declarations)
endif ()
endif ()

# GTest doesn't detect <tuple> with clang.
Expand Down
17 changes: 13 additions & 4 deletions test/format-test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2491,18 +2491,27 @@ TEST(FormatTest, FmtStringInTemplate) {

#endif // FMT_USE_CONSTEXPR

// C++20 feature test, since r346892 Clang considers char8_t a fundamental
// type in this mode. If this is the case __cpp_char8_t will be defined.
#ifndef __cpp_char8_t
// A UTF-8 code unit type.
#define CHAR8_T fmt::char8_t
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest replacing a macro with a using declaration.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

With c++11 using, it looks way way better now. Thank you.

#else
#define CHAR8_T char8_t
#endif

TEST(FormatTest, ConstructU8StringViewFromCString) {
fmt::u8string_view s("ab");
EXPECT_EQ(s.size(), 2u);
const fmt::char8_t* data = s.data();
const CHAR8_T* data = s.data();
EXPECT_EQ(data[0], 'a');
EXPECT_EQ(data[1], 'b');
}

TEST(FormatTest, ConstructU8StringViewFromDataAndSize) {
fmt::u8string_view s("foobar", 3);
EXPECT_EQ(s.size(), 3u);
const fmt::char8_t* data = s.data();
const CHAR8_T* data = s.data();
EXPECT_EQ(data[0], 'f');
EXPECT_EQ(data[1], 'o');
EXPECT_EQ(data[2], 'o');
Expand All @@ -2513,7 +2522,7 @@ TEST(FormatTest, U8StringViewLiteral) {
using namespace fmt::literals;
fmt::u8string_view s = "ab"_u;
EXPECT_EQ(s.size(), 2u);
const fmt::char8_t* data = s.data();
const CHAR8_T* data = s.data();
EXPECT_EQ(data[0], 'a');
EXPECT_EQ(data[1], 'b');
EXPECT_EQ(format("{:*^5}"_u, "🤡"_u), "**🤡**"_u);
Expand All @@ -2536,6 +2545,6 @@ TEST(FormatTest, CharTraitsIsNotAmbiguous) {
char_traits<char>::char_type c;
#if __cplusplus >= 201103L
std::string s;
begin(s);
auto lval = begin(s);
#endif
}
7 changes: 7 additions & 0 deletions test/posix-mock-test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -461,6 +461,10 @@ struct LocaleMock {
# ifdef _MSC_VER
# pragma warning(push)
# pragma warning(disable : 4273)
# ifdef __clang__
# pragma clang diagnostic push
# pragma clang diagnostic ignored "-Winconsistent-dllimport"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you post the warnings?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

C:\Users\User\AppData\Roaming\fmt>ninja -j 2
[4/47] Linking CXX shared library bin\fmt.dll
   Creating library fmt.lib and object fmt.exp
[35/47] Building CXX object test\CMakeFiles\posix-mock-test.dir\posix-mock-test.cc.obj
C:\Users\User\AppData\Roaming\fmt-master\test\posix-mock-test.cc(469,11): warning: '_create_locale' redeclared without 'dllimport' attribute: 'dllexport' attribute added [-Winconsistent-dllimport]
_locale_t _create_locale(int category, const char* locale) {
          ^
C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\ucrt\locale.h(105,32): note: previous declaration is here
    _ACRTIMP _locale_t __cdecl _create_locale(
                               ^
C:\Users\User\AppData\Roaming\fmt-master\test\posix-mock-test.cc(473,6): warning: '_free_locale' redeclared without 'dllimport' attribute: 'dllexport' attribute added [-Winconsistent-dllimport]
void _free_locale(_locale_t locale) {
     ^
C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\ucrt\locale.h(110,27): note: previous declaration is here
    _ACRTIMP void __cdecl _free_locale(
                          ^
C:\Users\User\AppData\Roaming\fmt-master\test\posix-mock-test.cc(477,8): warning: '_strtod_l' redeclared without 'dllimport' attribute: 'dllexport' attribute added [-Winconsistent-dllimport]
double _strtod_l(const char* nptr, char** endptr, _locale_t locale) {
       ^
C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\ucrt\stdlib.h(504,25): note: previous declaration is here
_ACRTIMP double __cdecl _strtod_l(
                        ^
3 warnings generated.

# endif

_locale_t _create_locale(int category, const char* locale) {
return LocaleMock::instance->newlocale(category, locale, 0);
Expand All @@ -473,6 +477,9 @@ void _free_locale(_locale_t locale) {
double _strtod_l(const char* nptr, char** endptr, _locale_t locale) {
return LocaleMock::instance->strtod_l(nptr, endptr, locale);
}
# ifdef __clang__
# pragma clang diagnostic pop
# endif
# pragma warning(pop)
# endif

Expand Down