Page MenuHomePhabricator

[TSan] Use switches when dealing with enums
ClosedPublic

Authored by yln on Thu, Jan 3, 3:11 PM.

Details

Summary

Small refactoring: replace some if-else cascades with switches so that the compiler warns us about missing cases.
Maybe found a small bug?

Diff Detail

Repository
rL LLVM

Event Timeline

yln created this revision.Thu, Jan 3, 3:11 PM
Herald added subscribers: Restricted Project, llvm-commits. · View Herald Transcript
yln marked an inline comment as done.Thu, Jan 3, 3:12 PM
yln added inline comments.
lib/tsan/rtl/tsan_suppressions.cc
96 ↗(On Diff #180149)

Should this be kSuppressionSignal?

dvyukov accepted this revision.Fri, Jan 4, 1:10 AM
This revision is now accepted and ready to land.Fri, Jan 4, 1:10 AM
delcypher added inline comments.Fri, Jan 4, 5:24 AM
lib/tsan/rtl/tsan_debugging.cc
41 ↗(On Diff #180149)

@yln I'd feel slightly happier by having UNREACHABLE("Unhandled case"); after the switch block (i.e. don't add a default case) so that if we do fall through (because someone missed a compiler warning, or perhaps they have warnings turned off) we stop execution rather than having undefined behaviour (reach end of function but no return statement).

This applies to all the switch blocks you've added.

I won't insist on this but if you think this a reasonable change that doesn't clutter things too much then I think you should do it.

yln updated this revision to Diff 180262.Fri, Jan 4, 9:31 AM

Add UNREACHABLE("missing case");

yln marked 2 inline comments as done.Fri, Jan 4, 9:36 AM
yln added inline comments.
lib/tsan/rtl/tsan_debugging.cc
41 ↗(On Diff #180149)

Good point! Thanks for the review, Dan.

lib/tsan/rtl/tsan_suppressions.cc
96 ↗(On Diff #180149)

@dvyukov @kubamracek: can you please confirm that this is intentional and not an oversight?

yln marked an inline comment as done.Fri, Jan 4, 9:37 AM
This revision was automatically updated to reflect the committed changes.