Skip to content

Conversation

@taminomara
Copy link
Contributor

@taminomara taminomara commented Jun 5, 2025

This commit adds CTTokenMapBuilder that replaces ct_token_map. It provides a few new features:

  • generated module is no longer #[allow(dead_code)] unless manually enabled. This helps to spot errors in custom lexers where some of the tokens are never emitted;

  • if generated module contains invalid identifiers, the generation will fail and suggest using CTTokenMapBuilder::rename_map;

  • CTTokenMapBuilder::rename_map accepts any iterable that yields pairs of strings, eliminating the need to convert static arrays of pairs of strings before passing them to builder.

The ct_token_map function is deprecated since the next minor release (0.14).

@ratmice
Copy link
Collaborator

ratmice commented Jun 5, 2025

I haven't yet really had the need to use a custom lexer (though I've thought about doing so for implementing rusts raw strings), so I don't really have much of an opinion on the API.

One thing I'll note is that in lrpar we allow the user to specify inner attributes directly within the module,
The code in question is here https://github.com/softdevteam/grmtools/blob/master/lrpar/src/lib/ctbuilder.rs#L963C1-L964C30

I don't remember/couldn't find where this would be tested,

%%
Start -> (): ... { () };
%%
// In first line of the place where helper functions go will be embedded within a module:
#![allow(foo)] // Because this is at the start of the module we can embed inner attributes enabling/disabling lints here.

I wonder then if it isn't worth doing something similar here,
instead of setting a boolean flag which specifies whether a specific lint should be enabled,
allow a String parameter to specify inner attributes within the generated code directly,
and then if you don't want the default lint set you could specify the empty string?

I don't know if that is better than specifying a boolean flag or not, but perhaps it might help future proofing the case where some new clippy lint starts complaining. Then users wouldn't necessarily have to wait until we add an allow to the default allowed lints and do a release. enabling them to just disable the lint themselves?

@taminomara
Copy link
Contributor Author

I wonder then if it isn't worth doing something similar here,
instead of setting a boolean flag which specifies whether a specific lint should be enabled,
allow a String parameter to specify inner attributes within the generated code directly,
and then if you don't want the default lint set you could specify the empty string?

I don't have a strong opinion here, either approach is fine. I'll try to make a builder interface, though. This will be cleaner than passing booleans around 😄

@ratmice
Copy link
Collaborator

ratmice commented Jun 5, 2025

I don't have a strong opinion here

Just for the record, I don't either it was just a thought.

@taminomara
Copy link
Contributor Author

Just for the record, I don't either it was just a thought.

I tried the builder pattern, and it was clunky and not that useful in this situation. I propose leaving things as is.

@ltratt
Copy link
Member

ltratt commented Jun 6, 2025

ct_token_map was always a mildly strange function to me (and I'm the person responsible for it!). I can imagine that other people might want other settings when generating the module, so I do think a simple builder is the right thing to go for here, rather than set ourselves up for a proliferation of similarly named functions. We can still keep (and mark as deprecated) ct_token_map so people have a simple upgrade path.

I would think that something as simple as CTTokenMapBuilder::new(mod_name, token_map, rename_map).silence_dead_code(false).build() or similar might do the trick, if everyone's amenable.

@ltratt ltratt self-assigned this Jun 6, 2025
@taminomara taminomara changed the title Add version of ct_token_map that doesn't insert #[allow(unused)] Replace ct_token_map with CTTokenMapBuilder Jun 7, 2025
@taminomara taminomara force-pushed the master branch 2 times, most recently from fc2f935 to 091115f Compare June 8, 2025 12:35
@taminomara
Copy link
Contributor Author

I believe everything is fixed 👌🏻

@ltratt
Copy link
Member

ltratt commented Jun 8, 2025

One minor comment, and then I think we're good to go.

This commit adds `CTTokenMapBuilder` that replaces `ct_token_map`. It provides a few new features:

- generated module is no longer `#[allow(dead_code)]` unless manually enabled. This helps to spot errors in custom lexers where some of the tokens are never emitted;

- if generated module contains invalid identifiers, the generation will fail and suggest using `CTTokenMapBuilder::rename_map`;

- `CTTokenMapBuilder::rename_map` accepts any iterable that yields pairs of strings, eliminating the need to convert static arrays of pairs of strings before passing them to builder.

The `ct_token_map` function is deprecated since the next minor release (`0.14`).
@ltratt
Copy link
Member

ltratt commented Jun 9, 2025

Thanks!

@ltratt ltratt added this pull request to the merge queue Jun 9, 2025
Merged via the queue into softdevteam:master with commit 5895724 Jun 9, 2025
2 checks passed
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.

3 participants