Page MenuHomePhabricator

Guard a feature that unsupported by old GCC
ClosedPublic

Authored by twoh on Feb 6 2019, 2:47 PM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

twoh created this revision.Feb 6 2019, 2:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 6 2019, 2:47 PM
kkwli0 added a subscriber: kkwli0.Feb 6 2019, 3:07 PM

See https://reviews.llvm.org/D57848. I think we can close either one.

twoh added a comment.Feb 6 2019, 3:11 PM

@kkwli0 Thanks for letting me know! But wouldn't https://reviews.llvm.org/D57848 always fall to "else" case added if the compiler is not GCC?

@kkwli0 Thanks for letting me know! But wouldn't https://reviews.llvm.org/D57848 always fall to "else" case added if the compiler is not GCC?

I agree, Clang defines __GNUC__ to value 4 for compatibility reasons. Likewise, this change doesn't handle compilers other than GCC that don't support __has_cpp_attribute (yet).

I'd propose to take the solution that libcxx, LLVMSupport, and LLVMDemangle use:

#ifndef __has_cpp_attribute
#define __has_cpp_attribute(__x) 0
#endif

Also Kelvin is right that _GNUC_VER is not defined here (it's from libcxx). Maybe we should take the macros from LLVM (except the non-C++ case)? That would also give us a correct checking for gnu::fallthrough, but remove __attribute__((__fallthrough__)) completely.

kkwli0 added a comment.Feb 7 2019, 9:18 AM

I close https://reviews.llvm.org/D57848 and consolidate the review and patch in this one.

twoh updated this revision to Diff 185914.Feb 7 2019, 11:59 PM

Addressing comments from @Hahnfeld. Thanks!

Hahnfeld accepted this revision.Feb 8 2019, 5:12 AM

Excellent!

This revision is now accepted and ready to land.Feb 8 2019, 5:12 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 8 2019, 9:16 AM