Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue 364 #365

Closed
wants to merge 2 commits into from
Closed

Issue 364 #365

wants to merge 2 commits into from

Conversation

ratmice
Copy link
Collaborator

@ratmice ratmice commented Nov 2, 2022

I'm not sure if we want to support rust_2018 in generated code, if we do though here is a patch
with this you can add #![deny(rust_2018_idioms)] to the ast_example from the book, and it should compile.

Above and beyond that, if you add #![deny(rust_2018_idioms)] to the generated code module, it brings up a couple of more errors this fixes those too.

Unfortunately, I'm not exactly sure a good way to test this, as most of the time rust emits: warning: deny(rust_2018_idioms) is ignored unless specified at crate level.

There were also some general issues with the ast_example regarding the calc.l names '*' vs 'MUL', missing type parameters, etc.
so if we decide not to support rust_2018_idioms, I should remember to break that out of this patch.

@ltratt
Copy link
Member

ltratt commented Nov 2, 2022

Hmm, this is definitely ugly:

-fn eval(lexer: &dyn NonStreamingLexer<u32>, e: Expr) -> Result<u64, (Span, &'static str)> {
+fn eval(lexer: &dyn NonStreamingLexer<'_, DefaultLexeme, u32>, e: Expr) -> Result<u64, (Span, &'static str)> {

My initial feeling is that, since the docs are aimed at new readers, they'll mostly be using whatever the latest and greatest Rust edition is. But, over time, it's quite possible that Rust editions will change more and more things and we'll have an increasing tension between "support old editions" and "make things nice in the latest edition".

On that basis, I think we should probably set a policy along the lines of:

  1. The "main" docs should aim to support as many editions as possible, provided that doesn't make things ugly. [The example above is an unfortunate example of ugliness in an old edition!]
  2. There should be a section in the docs along the lines of "Rust editions" which tells people how to adjust things for older Rust editions.
  3. CTParserBuilder and CTLexerBuilder should grow an edition(...) function which says what Rust edition they should generate code for.

Any thoughts?

@ratmice
Copy link
Collaborator Author

ratmice commented Nov 2, 2022

Hmm, this is definitely ugly:

-fn eval(lexer: &dyn NonStreamingLexer<u32>, e: Expr) -> Result<u64, (Span, &'static str)> {
+fn eval(lexer: &dyn NonStreamingLexer<'_, DefaultLexeme, u32>, e: Expr) -> Result<u64, (Span, &'static str)> {

Just wanted to state (if it isn't obvious), that the 2018 idiom here, is just the addition of '_, The additional type parameter
is fallout from d586d36 where the calc_ast example got updated but docs/ seemed to get missed. This could probably use a further look through to see if there are subsequent changes to bring them in line with one another. But that beyond the scope of this patch.

My initial feeling is that, since the docs are aimed at new readers, they'll mostly be using whatever the latest and greatest Rust edition is. But, over time, it's quite possible that Rust editions will change more and more things and we'll have an increasing tension between "support old editions" and "make things nice in the latest edition".

On that basis, I think we should probably set a policy along the lines of:

1. The "main" docs should aim to support as many editions as possible, provided that doesn't make things ugly. [The example above is an unfortunate example of ugliness in an old edition!]

2. There should be a section in the docs along the lines of "Rust editions" which tells people how to adjust things for older Rust editions.

3. `CTParserBuilder` and `CTLexerBuilder` should grow an `edition(...)` function which says what Rust edition they should generate code for.

Any thoughts?

This all seems reasonable, I initially wasn't so convinced of 3, but I think it does at least help make it obvious in the generator why certain idioms are there and prevent their accidental removal during cleanup, so I've come around to it.

@ltratt
Copy link
Member

ltratt commented Nov 2, 2022

Why don't we do this in (at least) two stages? In this PR let's just do the minimal fixes to bring the current documentation up to scratch. Then we can do another PR where we do e.g. the CTParserBuilder thing and add the new "Rust editions" section to the book.

@ratmice
Copy link
Collaborator Author

ratmice commented Nov 2, 2022

Sounds good, I'll open a new PR for this once we've got the documentation up to date.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants