Don't treat pointers as data. #911
Conversation
src/front/wgsl/mod.rs
Outdated
| } | ||
| _ => false, | ||
| }; | ||
| let mut needs_deref = allow_deref |
There was a problem hiding this comment.
Is this really better than the old code?
I think we should only use macros where their benefit outweight the cost of having an extra layer of indirection. Macros slow down compilation, confuse IDEs, and generally shouldn't be used so sparingly.
There was a problem hiding this comment.
Okay, this is the case where I think matches! actually works. Putting allow_deref in the first arm of the match is a really obscure way of saying &&. And if you do use &&, then the match just has arms with => true and => false, which is classic matches!. Since this is a std macro, I think IDEs are going to pick up on it pretty quickly; rust-analyzer, at least, hasn't been having any problems with it.
But if you don't find that persuasive, it's not something I feel strongly about.
There was a problem hiding this comment.
I think I agree. It's not striking me as a case that would need to be an exception though if we try to avoid matches in general (I checked briefly and didn't find a clippy for this).
src/valid/expression.rs
Outdated
| @@ -755,6 +755,9 @@ impl super::Validator { | |||
| if !condition_good || accept_inner != reject_inner { | |||
| return Err(ExpressionError::InvalidSelectTypes); | |||
| } | |||
| if let Ti::Pointer { .. } = *accept_inner { | |||
There was a problem hiding this comment.
I think this is a wrong level to check this. We are checking for pointers, but what if it's a texture, for example?
Instead, we need to check the derived type flags on this and ensure this is DATA | SIZED
|
@jimblandy what is the status of this PR? |
|
Probably stale. It needs to be reevaluated in light of the current state of the language spec and the code, which has evolved. |
|
The easiest thing to do would be to demote it (re-file it) as an issue instead of a PR. |
jimblandy
force-pushed
the
pointer-not-data
branch
from
8968606
to
4346d13
Compare
|
Converted to draft - just realized there were other commits on the branch I hadn't noticed, and whose changes need to be incorporated. |
Introduce a new `TypeFlags::CONSTRUCTIBLE` flag, corresponding to WGSL's "constructible types". Set this on the appropriate types. Check for this flag on function return types.
jimblandy
force-pushed
the
pointer-not-data
branch
from
4346d13
to
4eae569
Compare
All commits should be CI-clean.
Don't allow pointers in return values.
[wgsl-in]: Use a
matches!expression for brevity.Don't permit
Expression::Selectto operate on pointers. At the moment, Nagadoesn't support anything like SPIR-V's 'variable pointers'.
Naga
TypeInner::Pointertypes are notTypeFlags::DATA.Change
Validator::validate_typenot to mark pointers with theTypeFlags::Dataflag. TheTypeFlags::DATAflag is supposed to correspond toWGSL's concept of a 'plain type': a scalar, atomic, or composite type, excluding
textures, samplers, and pointers.
With this change, there is no type where
TypeFlags::DATAandTypeFlags::INTERFACEdiffer, but I'm not removing anything in this PR, becauseI think we'll want to re-introduce a different distinction between them soon.