Add handle validation pass to `Validator` #2090

ErichDonGubler · 2022-10-19T02:05:16Z

Resolves #1692. Recommended review experience is commit-by-commit.

ErichDonGubler · 2022-10-25T23:36:08Z

@jimblandy: I think that this is finally ready! 🙌🏻

ErichDonGubler · 2022-11-01T17:28:18Z

@jimblandy: Just wanted to ping here again, and get confirmation on whether reviewing this is a priority for you or not. Whatever the case, thanks in advance! 🙂

jimblandy

Okay, this looks great - just what we need. There are a few omissions that need to be addressed, and I had some suggestions.

src/proc/mod.rs

src/valid/analyzer.rs

src/valid/expression.rs

src/valid/handles.rs

src/valid/mod.rs

Related: <gfx-rs#2090 (comment)>

Requested at <gfx-rs#2090 (comment)>.

…r::validate` Resolves <gfx-rs#2090 (comment)>.

ErichDonGubler · 2022-12-13T15:58:53Z

Did some further clippy warning hunting with a02b02b, 00f4f6b, ce6f85e, and 6493235. EDIT: CI is green, yay!

ErichDonGubler · 2022-12-13T16:19:59Z

@jimblandy: Thanks for the thorough feedback. Once you've had a chance to look through the round of fixes I've pushed up here, LMK, and I'll squash things down into a sane set of commits again. Everything squashes down cleanly, so 😤.

jimblandy

When our CI runs:

cargo check -p naga --no-default-features

it gets a lot of warnings about unused functions, etc. in src/valid/handles.rs. We should put #[cfg(feature = "validate")] in the appropriate places (perhaps on the entire valid::handles module?) to make the non-validating configuration compile cleanly again.

src/valid/handles.rs

jimblandy

I'll need to get back to the rest of this tomorrow, but here are some comments so far.

src/valid/handles.rs

src/proc/mod.rs

…r::validate` Intended to resolve <gfx-rs#2090 (comment)>.

…r::validate` Should implement <gfx-rs#2090 (comment)>.

ErichDonGubler · 2022-12-13T22:30:43Z

@jimblandy:

When our CI runs … it gets a lot of warnings about unused functions, etc. in src/valid/handles.rs. We should put #[cfg(feature = "validate")] in the appropriate places (perhaps on the entire valid::handles module?) to make the non-validating configuration compile cleanly again.

Silenced warnings with 113957a.

Related: <gfx-rs#2090 (comment)>

Requested at <gfx-rs#2090 (comment)>.

ErichDonGubler · 2022-12-15T19:18:55Z

I've autosquashed all reviewed content into a few (hopefully nice) commits. Now that I've rebased onto master, I'm getting unrelated CI failures. I've submitted #2176 to resolve them.

CC @jimblandy.

src/valid/handles.rs

…r::validate` Resolves <gfx-rs#2090 (comment)>.

…dle::new`

…date`

Related: <gfx-rs#2090 (comment)>

Requested at <gfx-rs#2090 (comment)>.

…r::validate`

jimblandy

Looks great! Just two small changes, and then I think we're ready to go.

src/arena.rs

src/valid/expression.rs

Undo some changes from gfx-rs#1668, now that gfx-rs#2090 has been merged.

Undo some changes from #1668, now that #2090 has been merged.

ErichDonGubler mentioned this pull request Oct 19, 2022

UPSTREAM: Add handle validation pass ErichDonGubler/naga#2

Closed

ErichDonGubler force-pushed the handle-validation branch 5 times, most recently from 7a3d209 to fb43860 Compare October 25, 2022 23:33

ErichDonGubler marked this pull request as ready for review October 25, 2022 23:34

ErichDonGubler force-pushed the handle-validation branch 2 times, most recently from 6b3dab0 to a040828 Compare October 27, 2022 16:51

jimblandy force-pushed the handle-validation branch from a040828 to 4b87527 Compare December 9, 2022 19:31

jimblandy requested changes Dec 10, 2022

View reviewed changes

ErichDonGubler added a commit to ErichDonGubler/naga that referenced this pull request Dec 13, 2022

docs: s/if the constants/if constants/

c533142

Related: <gfx-rs#2090 (comment)>

ErichDonGubler added a commit to ErichDonGubler/naga that referenced this pull request Dec 13, 2022

docs: remove Panics section of TypeInner::size

65251d6

Requested at <gfx-rs#2090 (comment)>.

ErichDonGubler added a commit to ErichDonGubler/naga that referenced this pull request Dec 13, 2022

fixup! refactor: split handle validation into early pass in `Validato…

f2f8e40

…r::validate` Resolves <gfx-rs#2090 (comment)>.

jimblandy requested changes Dec 13, 2022

View reviewed changes

src/valid/handles.rs Outdated Show resolved Hide resolved

src/valid/handles.rs Outdated Show resolved Hide resolved

src/valid/handles.rs Outdated Show resolved Hide resolved

jimblandy reviewed Dec 13, 2022

View reviewed changes

src/valid/handles.rs Outdated Show resolved Hide resolved

src/valid/handles.rs Show resolved Hide resolved

src/proc/mod.rs Outdated Show resolved Hide resolved

ErichDonGubler added a commit to ErichDonGubler/naga that referenced this pull request Dec 13, 2022

fixup! refactor: split handle validation into early pass in `Validato…

7f0faf9

…r::validate` Intended to resolve <gfx-rs#2090 (comment)>.

ErichDonGubler added a commit to ErichDonGubler/naga that referenced this pull request Dec 13, 2022

fixup! refactor: split handle validation into early pass in `Validato…

df755c3

…r::validate` Should implement <gfx-rs#2090 (comment)>.

ErichDonGubler added a commit to ErichDonGubler/naga that referenced this pull request Dec 15, 2022

docs: s/if the constants/if constants/

ea7f5b4

Related: <gfx-rs#2090 (comment)>

ErichDonGubler added a commit to ErichDonGubler/naga that referenced this pull request Dec 15, 2022

docs: remove Panics section of TypeInner::size

b4d4c24

Requested at <gfx-rs#2090 (comment)>.

ErichDonGubler force-pushed the handle-validation branch from df755c3 to b4d4c24 Compare December 15, 2022 18:43

jimblandy requested changes Dec 16, 2022

View reviewed changes

src/valid/handles.rs Outdated Show resolved Hide resolved

src/valid/handles.rs Outdated Show resolved Hide resolved

src/valid/handles.rs Outdated Show resolved Hide resolved

src/valid/handles.rs Outdated Show resolved Hide resolved

ErichDonGubler added a commit to ErichDonGubler/naga that referenced this pull request Dec 16, 2022

fixup! refactor: split handle validation into early pass in `Validato…

b43468a

…r::validate` Resolves <gfx-rs#2090 (comment)>.

ErichDonGubler added a commit to ErichDonGubler/naga that referenced this pull request Dec 16, 2022

fixup! refactor: split handle validation into early pass in `Validato…

af21150

…r::validate` Resolves <gfx-rs#2090 (comment)>.

ErichDonGubler added a commit to ErichDonGubler/naga that referenced this pull request Dec 16, 2022

fixup! refactor: split handle validation into early pass in `Validato…

62f43a6

…r::validate` Resolves <gfx-rs#2090 (comment)>.

ErichDonGubler added a commit to ErichDonGubler/naga that referenced this pull request Dec 16, 2022

fixup! refactor: split handle validation into early pass in `Validato…

e1229f1

…r::validate` Resolves <gfx-rs#2090 (comment)>.

ErichDonGubler added 4 commits December 16, 2022 16:56

refactor(arena): factor out BadHandle construction into new `BadHan…

4db4075

…dle::new`

refactor: split handle validation into early pass in `Validator::vali…

2e0c10b

…date`

docs: s/if the constants/if constants/

f51da08

Related: <gfx-rs#2090 (comment)>

docs: remove Panics section of TypeInner::size

0ab6cc1

Requested at <gfx-rs#2090 (comment)>.

ErichDonGubler force-pushed the handle-validation branch from 7e56ad7 to 0ab6cc1 Compare December 16, 2022 22:21

ErichDonGubler added 2 commits December 16, 2022 17:24

fixup! refactor: split handle validation into early pass in `Validato…

f5b280a

…r::validate`

fixup! refactor: split handle validation into early pass in `Validato…

ff7c7d4

…r::validate`

jimblandy requested changes Dec 16, 2022

View reviewed changes

src/arena.rs Outdated Show resolved Hide resolved

src/valid/expression.rs Show resolved Hide resolved

Keep arena::Range::inner field private.

60fc9f1

jimblandy approved these changes Dec 17, 2022

View reviewed changes

jimblandy enabled auto-merge (squash) December 17, 2022 00:25

jimblandy merged commit 461fdda into gfx-rs:master Dec 17, 2022

ErichDonGubler deleted the handle-validation branch December 19, 2022 14:34

jimblandy added a commit to jimblandy/naga that referenced this pull request Jan 10, 2023

Let the uniformity analysis trust the handle validation pass.

93090fb

Undo some changes from gfx-rs#1668, now that gfx-rs#2090 has been merged.

jimblandy mentioned this pull request Jan 10, 2023

Let the uniformity analysis trust the handle validation pass. #2200

Merged

jimblandy added a commit to jimblandy/naga that referenced this pull request Jan 11, 2023

Let the uniformity analysis trust the handle validation pass.

56262d0

Undo some changes from gfx-rs#1668, now that gfx-rs#2090 has been merged.

jimblandy added a commit to jimblandy/naga that referenced this pull request Jan 15, 2023

Let the uniformity analysis trust the handle validation pass.

d1577f5

Undo some changes from gfx-rs#1668, now that gfx-rs#2090 has been merged.

jimblandy added a commit that referenced this pull request Jan 15, 2023

Let the uniformity analysis trust the handle validation pass.

cdcfad2

Undo some changes from #1668, now that #2090 has been merged.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add handle validation pass to `Validator` #2090

Add handle validation pass to `Validator` #2090

ErichDonGubler commented Oct 19, 2022 •

edited

Loading

ErichDonGubler commented Oct 25, 2022

ErichDonGubler commented Nov 1, 2022

jimblandy left a comment

ErichDonGubler commented Dec 13, 2022 •

edited

Loading

ErichDonGubler commented Dec 13, 2022 •

edited

Loading

jimblandy left a comment

jimblandy left a comment

ErichDonGubler commented Dec 13, 2022

ErichDonGubler commented Dec 15, 2022 •

edited

Loading

jimblandy left a comment

Add handle validation pass to Validator #2090

Add handle validation pass to Validator #2090

Conversation

ErichDonGubler commented Oct 19, 2022 • edited Loading

ErichDonGubler commented Oct 25, 2022

ErichDonGubler commented Nov 1, 2022

jimblandy left a comment

Choose a reason for hiding this comment

ErichDonGubler commented Dec 13, 2022 • edited Loading

ErichDonGubler commented Dec 13, 2022 • edited Loading

jimblandy left a comment

Choose a reason for hiding this comment

jimblandy left a comment

Choose a reason for hiding this comment

ErichDonGubler commented Dec 13, 2022

ErichDonGubler commented Dec 15, 2022 • edited Loading

jimblandy left a comment

Choose a reason for hiding this comment

Add handle validation pass to `Validator` #2090

Add handle validation pass to `Validator` #2090

ErichDonGubler commented Oct 19, 2022 •

edited

Loading

ErichDonGubler commented Dec 13, 2022 •

edited

Loading

ErichDonGubler commented Dec 13, 2022 •

edited

Loading

ErichDonGubler commented Dec 15, 2022 •

edited

Loading