Page MenuHomePhabricator

[Compiler] Fix LLVM_NODISCARD for GCC
ClosedPublic

Authored by xbolva00 on Sep 17 2019, 8:50 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

xbolva00 created this revision.Sep 17 2019, 8:50 AM
xbolva00 edited the summary of this revision. (Show Details)Sep 17 2019, 8:51 AM
xbolva00 edited the summary of this revision. (Show Details)Sep 17 2019, 8:54 AM
aaron.ballman added inline comments.Sep 18 2019, 4:44 AM
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 marked an inline comment as done.Sep 18 2019, 4:59 AM
xbolva00 added inline comments.
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 added inline comments.Sep 18 2019, 5:05 AM
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 marked an inline comment as done.Sep 18 2019, 7:29 AM
xbolva00 added inline comments.
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 added inline comments.Sep 18 2019, 8:31 AM
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 marked an inline comment as done.Sep 18 2019, 9:04 AM
xbolva00 added inline comments.
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 updated this revision to Diff 220688.Sep 18 2019, 9:04 AM
xbolva00 edited the summary of this revision. (Show Details)
xbolva00 updated this revision to Diff 220693.Sep 18 2019, 9:28 AM
xbolva00 marked an inline comment as done.
xbolva00 edited the summary of this revision. (Show Details)
xbolva00 added inline comments.Sep 18 2019, 9:31 AM
include/llvm/Support/Compiler.h
140 ↗(On Diff #220514)

I know :) -pedantic.

Ok, I will do what you suggested.

xbolva00 marked an inline comment as done.Sep 23 2019, 3:09 PM
xbolva00 added inline comments.
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 accepted this revision.Sep 24 2019, 6:41 AM

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]].

This revision is now accepted and ready to land.Sep 24 2019, 6:41 AM
xbolva00 marked an inline comment as done.Sep 24 2019, 6:46 AM
xbolva00 added inline comments.
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 added inline comments.Sep 24 2019, 6:54 AM
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 marked an inline comment as done.Sep 24 2019, 6:57 AM
xbolva00 added inline comments.
include/llvm/Support/Compiler.h
150–151 ↗(On Diff #220693)

ok :)

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 24 2019, 6:59 AM