This is an archive of the discontinued LLVM Phabricator instance.

Differential D67663

[Compiler] Fix LLVM_NODISCARD for GCC
ClosedPublic

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

Details

Reviewers
jfb
rsmith
echristo
aaron.ballman
Commits
rGc526fcaed193: [Compiler] Fix LLVM_NODISCARD for GCC
rL372761: [Compiler] Fix LLVM_NODISCARD for GCC
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

Event Timeline

xbolva00 created this revision.Sep 17 2019, 8:50 AM
Herald added a subscriber: dexonsmith. · View Herald TranscriptSep 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
MaskRay added a subscriber: MaskRay.Sep 17 2019, 9:25 AM
xbolva00 added reviewers: echristo, aaron.ballman.Sep 17 2019, 6:57 PM
aaron.ballman added inline comments.Sep 18 2019, 4:44 AM
include/llvm/Support/Compiler.h
142

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
142

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
142

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
142

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
142

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
142

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
142

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

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

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

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

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

ok :)

Closed by commit rL372761: [Compiler] Fix LLVM_NODISCARD for GCC (authored by xbolva00). · Explain WhySep 24 2019, 6:59 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 24 2019, 6:59 AM
Herald added subscribers: llvm-commits, kristina. · View Herald Transcript

Revision Contents

PathSize
include/
llvm/
Support/
8 lines

Diff 220693

include/llvm/Support/Compiler.h

Show First 20 Lines • Show All 131 Lines • ▼ Show 20 Lines
#if __has_attribute(used) || LLVM_GNUC_PREREQ(3, 1, 0) #if __has_attribute(used) || LLVM_GNUC_PREREQ(3, 1, 0)
#define LLVM_ATTRIBUTE_USED __attribute__((__used__)) #define LLVM_ATTRIBUTE_USED __attribute__((__used__))
#else #else
#define LLVM_ATTRIBUTE_USED #define LLVM_ATTRIBUTE_USED
#endif #endif
/// LLVM_NODISCARD - Warn if a type or return value is discarded. /// 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) #if __cplusplus > 201402L && LLVM_HAS_CPP_ATTRIBUTE(nodiscard)
aaron.ballmanUnsubmitted
Not Done
ReplyInline Actions

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.

aaron.ballman: I'd like to understand why this line is the way it is. I have a hunch that someone put the…
xbolva00AuthorUnsubmitted
Done
ReplyInline Actions

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

xbolva00: Just to avoid nodiscard in C++11/14 (and avoid warnings - yes we could ditch the first check…
aaron.ballmanUnsubmitted
Not Done
ReplyInline Actions

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

aaron.ballman: Ooooh, yeah, I missed that in the intro text, sorry about that! I think the better approach is…
xbolva00AuthorUnsubmitted
Done
ReplyInline Actions

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 :)

xbolva00: No problem :) I made a mistake due to missing warning, but bots luckily caught it so I just…
aaron.ballmanUnsubmitted
Not Done
ReplyInline Actions

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?

aaron.ballman: Okay, if we're going to put in a temporary workaround, can you add comments to that line…
xbolva00AuthorUnsubmitted
Done
ReplyInline Actions

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: Oh sorry, I messed some things. I removed 'C++17' check #if LLVM_HAS_CPP_ATTRIBUTE(nodiscard)…
xbolva00AuthorUnsubmitted
Done
ReplyInline Actions

I know :) -pedantic.

Ok, I will do what you suggested.

xbolva00: I know :) -pedantic. Ok, I will do what you suggested.
#define LLVM_NODISCARD [[nodiscard]] #define LLVM_NODISCARD [[nodiscard]]
#elif LLVM_HAS_CPP_ATTRIBUTE(clang::warn_unused_result) #elif LLVM_HAS_CPP_ATTRIBUTE(clang::warn_unused_result)
#define LLVM_NODISCARD [[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
xbolva00AuthorUnsubmitted
Done
ReplyInline Actions

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

@aaron.ballman @rsmith

xbolva00: If you have a idea for better comment, please tell me. @aaron.ballman @rsmith
// 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]]
aaron.ballmanUnsubmitted
Not Done
ReplyInline Actions

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

aaron.ballman: I'd move this above the `[[clang::warn_unused_result]]` version; I think we want to prefer `…
xbolva00AuthorUnsubmitted
Done
ReplyInline Actions

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

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

?

xbolva00: This will cause a Clang warning, no? Or you want something like #elif defined(__GNUC__) && !
aaron.ballmanUnsubmitted
Not Done
ReplyInline Actions

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

aaron.ballman: Eh, on second thought, I don't think it's worth the extra conditional complexity.
xbolva00AuthorUnsubmitted
Done
ReplyInline Actions

ok :)

xbolva00: ok :)
#else #else
#define LLVM_NODISCARD #define LLVM_NODISCARD
#endif #endif
// Indicate that a non-static, non-const C++ member function reinitializes // 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 entire object to a known state, independent of the previous state of
// the object. // the object.
// //
▲ Show 20 Lines • Show All 426 Lines • Show Last 20 Lines