[clang] [clang:frontend] Move helper functions to common location for SemaSPIRV (PR #125045)

Muhammad Bassiouni via cfe-commits cfe-commits at lists.llvm.org
Thu Jan 30 23:05:43 PST 2025


bassiounix wrote:

>     1. If we were to introduce new helpers, then instead of making a new header for this, these functions should just become member functions of the `Sema` class.

`Sema` or `SemaBase`?
 
>     2. The new version of `CheckAllArgTypesAreCorrect()` is... way too complicated, in my opinion; I have trouble trying to figure out what two parameters that are a variant and a pair are supposed to mean from looking at the function, and it does feel like this function wants to be at least two or three separate functions...

I'll split the implementation in 2 functions.

> I think a better approach would be to leave the HLSL functions alone and just factor out a separate helper and put it in `SemaSPIRV.cpp`, because as-is, this pr isn’t exactly simplifying things...

Since there is a common functionalities, I think I'll go with the first approach, that is, declaring them in a common class.
This class is `Sema` for now as mentioned in the review.

https://github.com/llvm/llvm-project/pull/125045


More information about the cfe-commits mailing list