nemequ/hedley

nodiscard attribute is used for clang 3.9.1 when used via HEDLEY_WARN_UNUSED_RESULT but shouldn't

t-b opened this issue · 3 comments

t-b commented

We are seeing a build failure 1, 2 in nlohmann/json with the latest hedley version.

The message is

FAILED: /usr/bin/clang++-3.9  -DDOCTEST_CONFIG_SUPER_FAST_ASSERTS -Iinclude -I../test/thirdparty/doctest -I../test/thirdparty/fifo_map -I../single_include -Wno-deprecated -Wno-float-equal -Wall -Wextra -pedantic -Werror -std=gnu++11 -MD -MT test/CMakeFiles/test-conversions.dir/src/unit-conversions.cpp.o -MF test/CMakeFiles/test-conversions.dir/src/unit-conversions.cpp.o.d -o test/CMakeFiles/test-conversions.dir/src/unit-conversions.cpp.o -c ../test/src/unit-conversions.cpp

In file included from ../test/src/unit-conversions.cpp:36:

../single_include/nlohmann/json.hpp:16270:5: error: use of the 'nodiscard' attribute is a C++1z extension [-Werror,-Wc++1z-extensions]

    JSON_HEDLEY_WARN_UNUSED_RESULT

    ^

../single_include/nlohmann/json.hpp:1133:96: note: expanded from macro 'JSON_HEDLEY_WARN_UNUSED_RESULT'

    #define JSON_HEDLEY_WARN_UNUSED_RESULT JSON_HEDLEY_DIAGNOSTIC_DISABLE_CPP98_COMPAT_WRAP_([[nodiscard]])

I've tried reproducing it with pure hedley but failed.

Hacking it to work is pretty easy

$git diff .
diff --git a/single_include/nlohmann/json.hpp b/single_include/nlohmann/json.hpp
index 87222900..14d0c926 100644
--- a/single_include/nlohmann/json.hpp
+++ b/single_include/nlohmann/json.hpp
@@ -624,7 +624,8 @@ struct position_t
 #if \
     defined(__has_cpp_attribute) && \
     defined(__cplusplus) && \
-    (!defined(JSON_HEDLEY_SUNPRO_VERSION) || JSON_HEDLEY_SUNPRO_VERSION_CHECK(5,15,0))
+    (!defined(JSON_HEDLEY_SUNPRO_VERSION) || JSON_HEDLEY_SUNPRO_VERSION_CHECK(5,15,0)) && \
+    (!defined(__clang__) || JSON_HEDLEY_GCC_VERSION_CHECK(4,0,0))
     #define JSON_HEDLEY_HAS_CPP_ATTRIBUTE(attribute) __has_cpp_attribute(attribute)
 #else
     #define JSON_HEDLEY_HAS_CPP_ATTRIBUTE(attribute) (0)

but I'm not understanding the issue well enough to propose that hack as a PR.

Any ideas?

t-b commented

My hack is not correct. clang 4.0.0, see https://travis-ci.org/github/nlohmann/json/jobs/705709985, does also break.

t-b commented

I can reproduce it now with pure hedley:

$/rest/inst/llvm-3.9.1/bin/clang++ -Wno-deprecated -Wall -Wextra -pedantic -Werror -std=c++11 warn-unused-result.c -o warn-unused-result
warn-unused-result.c:21:1: error: use of the 'nodiscard' attribute is a C++1z extension [-Werror,-Wc++1z-extensions]
HEDLEY_WARN_UNUSED_RESULT
^
./../hedley.h:1013:84: note: expanded from macro 'HEDLEY_WARN_UNUSED_RESULT'
#  define HEDLEY_WARN_UNUSED_RESULT HEDLEY_DIAGNOSTIC_DISABLE_CPP98_COMPAT_WRAP_([[nodiscard]])
                                                                                   ^
warn-unused-result.c:26:1: error: use of the 'nodiscard' attribute is a C++1z extension [-Werror,-Wc++1z-extensions]
HEDLEY_WARN_UNUSED_RESULT_MSG("Foo")
^
./../hedley.h:1014:93: note: expanded from macro 'HEDLEY_WARN_UNUSED_RESULT_MSG'
#  define HEDLEY_WARN_UNUSED_RESULT_MSG(msg) HEDLEY_DIAGNOSTIC_DISABLE_CPP98_COMPAT_WRAP_([[nodiscard]])
                                                                                            ^
warn-unused-result.c:32:3: warning: ignoring return value of function declared with 'nodiscard' attribute [-Wunused-result]
  test_unused_result();
  ^~~~~~~~~~~~~~~~~~
warn-unused-result.c:33:3: warning: ignoring return value of function declared with 'nodiscard' attribute [-Wunused-result]
  test_unused_result_msg();
  ^~~~~~~~~~~~~~~~~~~~~~
2 warnings and 2 errors generated.

The -pedantic flag is the culprit.

nemequ commented

Thanks, sorry it took me so long to get to this.

It's a bit annoying that clang will emit a warning when using [[nodiscard]] even if we first check __has_cpp_attribute(nodiscard); I'm really not a fan of these -W*-extensions warnings. However, the problem is not unique to nodiscard and I do want to avoid all warnings, even the dumb ones, in Hedley.

I've reorganized the HEDLEY_WARN_UNUSED_RESULT macro to prefer __attribute__((__warn_unused_result__)) over [[nodiscard]], that should fix the problem.

I also added -Wc++1z-extensions to the list of warnings we disable in the HEDLEY_DIAGNOSTIC_DISABLE_CPP98_COMPAT_WRAP_ macro. It's a misleading name, it's really for all of these warnings; it's an internal (i.e., unstable) macro, hence the trailing underscore, but it's really quite useful, I'm thinking about renaming it and exposing it publicly. That should also fix the problem if it weren't for the first fix :)

I'm planning to put out a new release soon, this will be included.

  • 👍2