Page MenuHomePhabricator

Allow Compiler.h to be included in C files and fix fallthrough warnings
ClosedPublic

Authored by Nathan-Huckleberry on Thu, Aug 22, 11:11 AM.

Details

Summary

Since clang does not support comment style fallthrough annotations
these should be switched to macros defined in Compiler.h. This
requires some fixing to Compiler.h.

Original patch: https://reviews.llvm.org/D66487

Diff Detail

Repository
rL LLVM

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptThu, Aug 22, 11:11 AM
nickdesaulniers accepted this revision.Thu, Aug 22, 12:32 PM
This revision is now accepted and ready to land.Thu, Aug 22, 12:32 PM
aaron.ballman added a subscriber: rsmith.

Adding @rsmith as a reviewer just to be sure we are not missing something.

llvm/include/llvm/Support/Compiler.h
40–42 ↗(On Diff #216675)

I think that we want people to use LLVM_HAS_CPP_ATTRIBUTE instead of __has_cpp_attribute directly, so I think that this block should be removed.

48 ↗(On Diff #216675)

I would appreciate some comments explaining why this is needed so that someone doesn't come in and "fix" the code by removing the macro later without understanding why it's there.

49 ↗(On Diff #216675)

Then this can also check for defined(__has_cpp_attribute)

  • Add comment and remove direct use of __has_cpp_attribute
rsmith accepted this revision.Thu, Aug 22, 4:05 PM
rsmith added inline comments.
llvm/include/llvm/Support/Compiler.h
1–12 ↗(On Diff #216704)

The file comment should explicitly call out that we allow this function to be included in both C and C++ code.

44–45 ↗(On Diff #216704)

I don't think this comment makes it sufficiently clear what's going on here (I didn't understand until I saw the comment you deleted below). How about:

Only use __has_cpp_attribute in C++, because Clang 3.6 and before reject __has_cpp_attribute(scoped::attribute) in C.

or something like that?

  • Clarify comments
aaron.ballman accepted this revision.Fri, Aug 23, 5:28 AM

Aside from commenting issue, LGTM.

llvm/include/llvm/Support/Compiler.h
44–45 ↗(On Diff #216704)

I don't think this is just the Clang 3.6 issue though. At least part of the problem is that you cannot expand __has_cpp_attribute(clang::fallthrough) in C mode because of the :: in GCC. For some reason, GCC defines __has_cpp_attribute in C mode. https://godbolt.org/z/6JiwGv

Can we please either commit this, or revert r369414 otherwise? Several of the sanitizer buildbots have been red since Tuesday due to r369414.

aaron.ballman accepted this revision.Fri, Aug 23, 10:13 AM

LGTM, thank you for this!

nickdesaulniers accepted this revision.Fri, Aug 23, 10:20 AM
This revision was automatically updated to reflect the committed changes.