Fix build break in SemaHLSL.cpp on MSVC 2022: warning C4715: 'getResourceClass': not all control paths return a value

[dpaoliello]

Moves the existing llvm_unreachable statement to the bottom of the function and changes the case statement to deliberately fall through to it.

Build break was introduced by #111203

It was not caught by the builders as they use Visual Studio 2019, whereas this warning only appears in 2022.

[llvmbot]

@llvm/pr-subscribers-hlsl

@llvm/pr-subscribers-clang

Author: Daniel Paoliello (dpaoliello)

Changes

Adds a default case to the switch to the existing llvm_unreachable statement.

Build break was introduced by #111203

It was not caught by the builders as they use Visual Studio 2019, whereas this warning only appears in 2022.

---

Full diff: https://github.com/llvm/llvm-project/pull/112767.diff

1 Files Affected:

• (modified) clang/lib/Sema/SemaHLSL.cpp (+1)

diff --git a/clang/lib/Sema/SemaHLSL.cpp b/clang/lib/Sema/SemaHLSL.cpp
index efb0fbaa432d76..50f0a553bc86e3 100644
--- a/clang/lib/Sema/SemaHLSL.cpp
+++ b/clang/lib/Sema/SemaHLSL.cpp
@@ -102,6 +102,7 @@ static ResourceClass getResourceClass(RegisterType RT) {
     return ResourceClass::Sampler;
   case RegisterType::C:
   case RegisterType::I:
+  default:
     llvm_unreachable("unexpected RegisterType value");
   }
 }

[mizvekov]

There is a very useful warning about switching on an enum where not all enumerators are covered.

Adding a default case suppresses this warning, but this is undesirable, as we want to see this warning pop up when we add a new enumerator.

Suggested change

	default:
	llvm_unreachable("unexpected RegisterType value");
	}
	llvm_unreachable("unexpected RegisterType value");
	}
	llvm_unreachable("unhandled RegisterType");

[farzonl]

I don't agree with the suggested change of not using default.

@hekota says:

the switch covers all enum values.

It sounds like line 107 will be impossible to hit.

So then whats the difference between an unexpected and an unhandled register type? Why couldn't we have just done a default case?

If a new Register Type were added it would hit a llvm_unreachable on the default case and an implementer would quickly know to implement it from the call stack. I don't like the idea of changes to make a warning happy when that code change adds undistinguishable terminology between unexpected and unhandled. I would prefer not adding new terminology that would be confusing and instead add a default.

[dpaoliello]

It sounds like line 107 will be impossible to hit.

Not impossible: someone can always cast an arbitrary value into the enum type, which is what MSVC is trying to guard against.

As a compromise we could keep the unreachable at the bottom, and then deliberately fall through to that in the unhandled cases:

  case RegisterType::C:
  case RegisterType::I:
    // Deliberately falling through to the unreachable below.
    break;
  }
  llvm_unreachable("unexpected RegisterType value");
}

[AaronBallman]

As a compromise we could keep the unreachable at the bottom, and then deliberately fall through to that in the unhandled cases:

FWIW, that's the usual approach we take.

[farzonl]

I'm fine with that. We do that in getRegisterType.

[hekota]

LGTM, thank you for the fix! Interesting that it fires up, the switch covers all enum values.

[AaronBallman]

LGTM