This is an archive of the discontinued LLVM Phabricator instance.

[clang][Sema] Avoid duplicate diagnostics for unreachable fallthrough attribute

Authored by hazohelet on Mar 11 2023, 3:00 AM.



This patch checks whether -Wunreachable-code-fallthrough is enabled when clang encounters unreachable fallthrough attributes and, if so, suppresses code will never be executed warning to avoid duplicate warnings.
This fixes

Diff Detail

Event Timeline

hazohelet created this revision.Mar 11 2023, 3:00 AM
Herald added a project: Restricted Project. · View Herald Transcript
hazohelet requested review of this revision.Mar 11 2023, 3:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 11 2023, 3:01 AM
shafik added a subscriber: shafik.Mar 12 2023, 10:59 AM
shafik added inline comments.

Your passing through DiagnosticsEngine just to check if diag::warn_unreachable_fallthrough_attr is ignored. I think it would be better just to pass through a boolean flag. This would avoid one include as well.

hazohelet updated this revision to Diff 504501.Mar 12 2023, 8:45 PM

Address comments from @shafik

  • Pass bool flag UnreachableFallThroughDiagIsEnabled instead of DiagnosticsEngine
hazohelet marked an inline comment as done.Mar 12 2023, 8:46 PM
aaron.ballman added inline comments.Mar 13 2023, 6:12 AM

We don't typically use const on value types in the project, so dropping this for style consistency. You should make the same edit elsewhere in the patch as well.


Would it be cleaner (less changes) to pass a boolean into this callback to say "the statement had a fallthrough attribute on it" so that UnreachableCodeHandler::HandleUnreachable() can check to see whether the unreachable fallthrough diagnostic is enabled or not? It has access to a Sema object, so it should be able to do the work. FindUnreachableCode() would then not have to change its signature and it keeps the "should I suppress this diagnostic logic" nearby where we issue the diagnostic along with the other duplicate handling logic.

hazohelet updated this revision to Diff 504697.Mar 13 2023, 9:18 AM

Address comments from @aaron.ballman

  • Dropped const qualifier from value objects for style consistency
  • Passed boolean HasFallThroughAttr to callback and handled warning suppression in HandleUnreachable in order to make code cleaner.

Thank you, I think this is heading in the right direction! Can you also add a release note to clang/docs/ReleaseNotes.rst about the fix?


NFC, just a stylistic choice.

Alternatively: change const AttributedStmt *AS = ... into const auto *AS = ...


Accidental whitespace change?

hazohelet updated this revision to Diff 504933.Mar 13 2023, 9:15 PM

Address comments from @aaron.ballman

  • NFC stylistic changes

Thanks for the review! Since I don't have commit access, would you please land this patch for me?
Please use "Takuya Shimizu <>" for the patch attribution.

This revision is now accepted and ready to land.Mar 14 2023, 5:42 AM