Page MenuHomePhabricator

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

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



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:

Diff Detail


Event Timeline

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

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

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.Aug 22 2019, 4:05 PM
rsmith added inline comments.
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.Aug 23 2019, 5:28 AM

Aside from commenting issue, LGTM.

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.

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.Aug 23 2019, 10:13 AM

LGTM, thank you for this!

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