Stream: git-wasmtime

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

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.

Wasmtime GitHub notifications bot (Feb 02 2023 at 23:34):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Feb 02 2023 at 23:34):

jameysharp submitted PR review.

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.

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'-') => {

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.

Wasmtime GitHub notifications bot (Feb 04 2023 at 17:39):

bjorn3 updated PR #5693 from isle_cleanup1 to main.

Wasmtime GitHub notifications bot (Feb 04 2023 at 17:39):

bjorn3 submitted PR review.

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.

Wasmtime GitHub notifications bot (Feb 04 2023 at 17:39):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Feb 04 2023 at 17:39):

bjorn3 created PR review comment:

It does. Changed it.

Wasmtime GitHub notifications bot (Feb 04 2023 at 17:40):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Feb 04 2023 at 17:40):

bjorn3 created PR review comment:

Changed to return Ok(None) on EOF.

Wasmtime GitHub notifications bot (Feb 04 2023 at 18:16):

bjorn3 updated PR #5693 from isle_cleanup1 to main.

Wasmtime GitHub notifications bot (Feb 06 2023 at 23:10):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Feb 06 2023 at 23:11):

jameysharp merged PR #5693.

Last updated: Nov 22 2024 at 17:03 UTC