Stream: git-wasmtime

Topic: wasmtime / PR #5693 Some refactorings to the ISLE parser


view this post on Zulip Wasmtime GitHub notifications bot (Feb 02 2023 at 16:36):

bjorn3 opened PR #5693 from isle_cleanup1 to main:

This uses a couple of functions in the ISLE lexer than significantly simply the code.

In addition it introduces a token eating api which tries to eat a specific token but returns None rather than an error if this fails and it renames take to expect. This brings it a bit more in line with the names used in rustc's parser and IMHO it is a bit nicer.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 02 2023 at 23:34):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 02 2023 at 23:34):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 02 2023 at 23:34):

jameysharp created PR review comment:

This can just be let infallible = self.eat_sym_str("infallible")?; now, in yet another demonstration of why your suggested change is a good idea.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 02 2023 at 23:34):

jameysharp created PR review comment:

Does Rust allow using the following syntax instead, and would you agree it's easier to read?

            c @ (b'0'..=b'9' | b'-') => {

view this post on Zulip Wasmtime GitHub notifications bot (Feb 02 2023 at 23:34):

jameysharp created PR review comment:

I would have expected eat to return Some(token) if there is a next token and it matches the predicate, and on the other hand, return None if there's no more tokens or the next token doesn't match. I'm not convinced that returning a Result in order to distinguish the EOF case makes sense here.

It doesn't exactly matter since everywhere this is used we expect more tokens to follow anyway, so it's okay to detect the EOF here, but I think this is simpler to use and its behavior is easier to explain if it simply returns None at EOF.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 04 2023 at 17:39):

bjorn3 updated PR #5693 from isle_cleanup1 to main.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 04 2023 at 17:39):

bjorn3 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 04 2023 at 17:39):

bjorn3 created PR review comment:

Indeed. Fixed a couple of other cases like this too.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 04 2023 at 17:39):

bjorn3 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 04 2023 at 17:39):

bjorn3 created PR review comment:

It does. Changed it.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 04 2023 at 17:40):

bjorn3 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 04 2023 at 17:40):

bjorn3 created PR review comment:

Changed to return Ok(None) on EOF.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 04 2023 at 18:16):

bjorn3 updated PR #5693 from isle_cleanup1 to main.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 06 2023 at 23:10):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 06 2023 at 23:11):

jameysharp merged PR #5693.


Last updated: Dec 23 2024 at 13:07 UTC