[Compiler] Fix LLVM_NODISCARD for GCC

[xbolva00]

Summary

This branch is currently dead since we don't use C++17.
#if __cplusplus > 201402L && LLVM_HAS_CPP_ATTRIBUTE(nodiscard)
#define LLVM_NODISCARD [[nodiscard]]

This branch is Clang-only.
#elif LLVM_HAS_CPP_ATTRIBUTE(clang::warn_unused_result)
#define LLVM_NODISCARD [[clang::warn_unused_result]]

While we could use gnu variant [[gnu::warn_unused_result]], it is not ideal because it works only on functions.
/home/xbolva00/LLVM/llvm/include/llvm/ADT/ArrayRef.h:41:24: warning: ‘warn_unused_result’ attribute only applies to function types [-Wattributes]

GCC (checked 5,6,7,8) seems to enable [[nodiscard]] even in C++14 mode and does not produce warnings that nodiscard is C++17 feature. but Clang does - but we do not reach it due the code above. So it affects only GCC and does what we want.

[aaron.ballman]

include/llvm/Support/Compiler.h

140 ↗

(On Diff #220514)

I'd like to understand why this line is the way it is. I have a hunch that someone put the __cplusplus check in first, and the __has_cpp_attribute check was added later without removing the initial check. I think the correct fix is to remove the __cplusplus check, but I'd like to know if there's a valid reason for it to exist before we remove it.

[xbolva00]

include/llvm/Support/Compiler.h

140 ↗

(On Diff #220514)

Just to avoid nodiscard in C++11/14 (and avoid warnings - yes we could ditch the first check and modify cxxflags and add -Wno-c++17-extensions - but I dont like it personally).

[aaron.ballman]

include/llvm/Support/Compiler.h

140 ↗

(On Diff #220514)

Ooooh, yeah, I missed that in the intro text, sorry about that!

I think the better approach is to fix: https://bugs.llvm.org/show_bug.cgi?id=33518. It's been on my plate for a while, but I've not had the time to do that fix yet. :-( If you wanted to take it over, I'd be happy to review it (and if not, don't worry, I'll get to it eventually).

[xbolva00]

include/llvm/Support/Compiler.h

140 ↗

(On Diff #220514)

No problem :) I made a mistake due to missing warning, but bots luckily caught it so I just look what went wrong. Yeah, that approach is better and more general, but possibly (?) it could break some code that depends on current behaviour?

I can use this patch as a workaround for now, it is not so critical :)

[aaron.ballman]

include/llvm/Support/Compiler.h

140 ↗

(On Diff #220514)

Okay, if we're going to put in a temporary workaround, can you add comments to that line explaining that the __cplusplus test and the workaround you are adding should both be removed once PR33518 is resolved?

[xbolva00]

include/llvm/Support/Compiler.h

140 ↗

(On Diff #220514)

Oh sorry, I messed some things.

I removed 'C++17' check
#if LLVM_HAS_CPP_ATTRIBUTE(nodiscard)
#define LLVM_NODISCARD [[nodiscard]]

It works fine with GCC 7. Good. But when I was testing this way, I got a warning so I ditched that way.

Seems like when I was trying to fix this macro in godbolt, I had to explicitely use the -Wc++17-extensions flag or what. Now I tried it again, and without -Wc++17-extensions I see no warnings.

https://godbolt.org/z/DjYLBF

[xbolva00]

include/llvm/Support/Compiler.h

140 ↗

(On Diff #220514)

I know :) -pedantic.

Ok, I will do what you suggested.

[xbolva00]

include/llvm/Support/Compiler.h

146 ↗

(On Diff #220693)

If you have a idea for better comment, please tell me.

@aaron.ballman @rsmith

[aaron.ballman]

LGTM with a small nit.

include/llvm/Support/Compiler.h

150–151 ↗

(On Diff #220693)

I'd move this above the [[clang::warn_unused_result]] version; I think we want to prefer [[nodiscard]].

[xbolva00]

include/llvm/Support/Compiler.h

150–151 ↗

(On Diff #220693)

This will cause a Clang warning, no? Or you want something like

#elif defined(GNUC) && !defined(clang) && LLVM_HAS_CPP_ATTRIBUTE(nodiscard)

?

[aaron.ballman]

include/llvm/Support/Compiler.h

150–151 ↗

(On Diff #220693)

Eh, on second thought, I don't think it's worth the extra conditional complexity.

[xbolva00]

include/llvm/Support/Compiler.h

150–151 ↗

(On Diff #220693)

ok :)

llvm/trunk/include/llvm/Support/Compiler.h

 
 #if __has_attribute(used) || LLVM_GNUC_PREREQ(3, 1, 0)
 #define LLVM_ATTRIBUTE_USED __attribute__((__used__))
 #else
 #define LLVM_ATTRIBUTE_USED
 #endif
 
 /// LLVM_NODISCARD - Warn if a type or return value is discarded.
+
+// Use the 'nodiscard' attribute in C++17 or newer mode.
 #if __cplusplus > 201402L && LLVM_HAS_CPP_ATTRIBUTE(nodiscard)
 #define LLVM_NODISCARD [[nodiscard]]
 #elif LLVM_HAS_CPP_ATTRIBUTE(clang::warn_unused_result)
 #define LLVM_NODISCARD [[clang::warn_unused_result]]
+// Clang in C++14 mode claims that it has the 'nodiscard' attribute, but also
+// warns in the pedantic mode that 'nodiscard' is a C++17 extension (PR33518).
+// Use the 'nodiscard' attribute in C++14 mode only with GCC.
+// TODO: remove this workaround when PR33518 is resolved.
+#elif defined(__GNUC__) && LLVM_HAS_CPP_ATTRIBUTE(nodiscard)
+#define LLVM_NODISCARD [[nodiscard]]
 #else
 #define LLVM_NODISCARD
 #endif
 
 // Indicate that a non-static, non-const C++ member function reinitializes
 // the entire object to a known state, independent of the previous state of
 // the object.
 //