Visual Studio 2017 warning

[petko]

In VS2017 (15.9.16), when used as part of nlohmann/json, I get the following error when using the library in my tests (it is reported as error, but the project builds OK):

Severity    Code    Description Project File    Line    Suppression State
Error (active)  E2512   the argument to a feature-test macro must be a simple identifier    Tests   [path]\json.hpp 1274    

This corresponds to LINE 1171 in your library: https://github.com/nemequ/hedley/blob/c395642679d4b79cd54d4f4a3ecb21fdf544513e/hedley.h#L1166-L1176

I hope it's not a duplicate, but haven't found it reported..

[nemequ]

Interesting, thank you for filing this instead of just ignoring it or fixing it locally. I'll have to get a VS2017 (and VS2019) build going on AppVeyor.

My guess is that this is due to the namespaced HEDLEY_HAS_CPP_ATTRIBUTE check (clang::fallthrough instead of fallthrough). PGI also has trouble with this, maybe it would be a good idea to add a HEDLEY_HAS_CPP_ATTRIBUTE_NS(ns, attribute) macro (as well as HEDLEY_GNUC_HAS_CPP_ATTRIBUTE and HEDLEY_GCC_HAS_CPP_ATTRIBUTE) so we can whitelist compilers which support namespaced attributes.

Another possibility could be to just disable the warning which something like

#define HEDLEY_HAS_CPP_ATTRIBUTE(attribute) \
  HEDLEY_DIAGNOSTIC_PUSH \
  __pragma(disable:2512) \
  __has_cpp_attribute(attribute) \
  HEDLEY_DIAGNOSTIC_POP

Although IIRC that won't work for PGI.

[beutlich]

It is an IntelliSense error only, right?

[petko]

It is an IntelliSense error only, right?

Yes, you are right. But it's annoying nevertheless..

[nemequ]

Oh, I didn't realize it was an IntelliSense thing. I'd still like to fix it, but do you know if there is a way to reproduce it from the CLI so we can test in CI? I'd like to make sure it doesn't happen again, but nothing is showing up in CI.

[t-b]

This is not a intellisense thing.

I do see the warning here with VS2017 as well if I use the default C++ language standard flag. If i use C++17 (/std:c++17) the warning is gone.

VS also supports namespaced attributes, see https://docs.microsoft.com/de-de/cpp/cpp/attributes?view=vs-2019 and look for gsl::suppress.

IMHO the check for clang::fallthrough should only be done for clang or?

@nemequ Are you compiling on CI on windows with /W4 /WX?

[nemequ]

I do see the warning here with VS2017 as well if I use the default C++ language standard flag. If i use C++17 (/std:c++17) the warning is gone.

With 2017 and no language version specified, it seems to be working in CI.

It also seems to work on godbolt 19.20+ (which is, I think, VS 2019). On VS 2017 (19.10+, godbolt has 19.14 - 19.16) __has_cpp_attribute doesn't seem to be defined, so HEDLEY_HAS_CPP_ATTRIBUTE(attribute) should be defined to (0).

IMHO the check for clang::fallthrough should only be done for clang or?

As a rule I'd like to avoid that if possible since compilers other than clang may implement clang-namespaced attributes in the future (look at what has happened with -webkit-* in CSS), but that may be the best option here.

CI is running with /Wall, so it should be triggering all the level 4 warnings, without any specific C++ version specified.

[t-b]

I've found https://forum.qt.io/topic/93711/problem-with-new-visual-studio-update-2017-version-15-8-0-and-qt-5-9-5/28 and they have basically the same problem.

I'm running VS 2017 15.9.16.

[t-b]

How about doing the following:

$ git diff .
diff --git a/hedley.h b/hedley.h
index 282a6c0..af91948 100644
--- a/hedley.h
+++ b/hedley.h
@@ -1168,10 +1168,12 @@ HEDLEY_DIAGNOSTIC_POP
      (__cplusplus >= 201703L) || \
      ((__cplusplus >= 201103L) && HEDLEY_HAS_CPP_ATTRIBUTE(fallthrough))
 #    define HEDLEY_FALL_THROUGH [[fallthrough]]
-#  elif (__cplusplus >= 201103L) && HEDLEY_HAS_CPP_ATTRIBUTE(clang::fallthrough)
-#    define HEDLEY_FALL_THROUGH [[clang::fallthrough]]
-#  elif (__cplusplus >= 201103L) && HEDLEY_GCC_VERSION_CHECK(7,0,0)
-#    define HEDLEY_FALL_THROUGH [[gnu::fallthrough]]
+#  elif !defined(HEDLEY_MSVC_VERSION)
+#    if (__cplusplus >= 201103L) && HEDLEY_HAS_CPP_ATTRIBUTE(clang::fallthrough)
+#      define HEDLEY_FALL_THROUGH [[clang::fallthrough]]
+#    elif (__cplusplus >= 201103L) && HEDLEY_GCC_VERSION_CHECK(7,0,0)
+#      define HEDLEY_FALL_THROUGH [[gnu::fallthrough]]
+#    endif
 #  endif
 #endif
 #if !defined(HEDLEY_FALL_THROUGH)
(base)

[nemequ]

I was thinking about something like that—though probably !defined(HEDLEY_MSVC_VERSION) || HEDLEY_MSVC_VERSION_CHECK(19,20,0) since VS 2019 seems fine—but I think I'd like to play with the HEDLEY_HAS_CPP_ATTRIBUTE_NS(ns, attribute) idea first. MSVC < 19.20 isn't the only compiler I've seen have trouble with namespaced attributes, so it would be nice to have a more generic solution.

IIRC the only reason that check for PGI is in the outer check is also the namespaced attributes, so that should probably be moved to where you have the MSVC check too.

I'll push something this weekend, one way or the other. This is pretty annoying for people using MSVC 2017, which is a pretty large number of people, so I'd like to release v10 soon too… maybe Tuesday or Wednesday.

[nemequ]

I believe this should be fixed in the dev branch (by 56e464db209276c8592522658dca22a08ff49044). Since I haven't been able to reproduce the problem, would those of you who are seeing this issue (@petko and @t-b, I guess?) please test?

Thanks for all the help on this one, I really appreciate it!

[petko]

Unfortunately, I do not use your library directly, but as a part of nlohmann/json, where it is amalgamated with a slightly different name, so I can't test it right away..

[nemequ]

Here is a quick fork with the updated version: https://github.com/nemequ/json/

If you just want the relevant commit, it's https://github.com/nemequ/json/commit/b47d866afa8d1696f39d7f71a2f22857101338e6

[petko]

I've tested it with your fork and it seems that it resolves the problem! Thanks!

[t-b]

@petko Nice! Could you create a pull request for the json project using https://github.com/nemequ/json/commit/b47d866afa8d1696f39d7f71a2f22857101338e6?

[nemequ]

@petko, awesome, thanks!

@t-b, that's the version from dev at the time, which isn't quite what will appear in the release. If you want I can update it after the release (probably in a couple hours) and submit a PR using that. FWIW, after the release it's just a make update_hedley (and maybe a quick s/gsed/sed/ in the Makefile, depending on your platform) to automatically pull in the updated version.

[t-b]

@nemequ

If you want I can update it after the release (probably in a couple hours) and submit a PR using that.

That would be awesome!

[nemequ]

Just released v11, this fix is included.

[nlohmann]

I love to see that this issue is fixed!!!