Skip to content

Public functions for Rule's internals #478

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.

Sign up for GitHub

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

Merged
ratmice merged 5 commits into from
Dec 21, 2024
Merged

Public functions for Rule's internals #478

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d4adf6f
Expose `tok_id` and `re_str` functions.
ltratt Dec 21, 2024
3c1ed4a
Add additional accessors for other "too public" fields.
ltratt Dec 21, 2024
641e798
Mark the `name` field as deprecated.
ltratt Dec 21, 2024
78406d0
Mark the `name_span` field as deprecated.
ltratt Dec 21, 2024
382eba9
Mark `start_states` and `target_state` as deprecated.
ltratt Dec 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 19 additions & 5 deletions lrlex/src/lib/lexer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,11 @@ pub const DEFAULT_REGEX_OPTIONS: RegexOptions = RegexOptions {
#[derive(Debug)]
#[doc(hidden)]
pub struct Rule<StorageT> {
/// If `Some`, the ID that lexemes created against this rule will be given (lrlex gives such
/// rules a guaranteed unique value, though that value can be overridden by clients who need to
/// control the ID). If `None`, then this rule specifies lexemes which should not appear in the
/// user's input.
/// If `Some`, this specifies the ID that lexemes resulting from this rule will have. Note that
/// lrlex gives rules a guaranteed unique value by default, though users can later override
/// that, potentially undermining uniqueness if they're not careful.
///
/// If `None`, then this rule specifies lexemes which should not appear in the user's input.
pub(super) tok_id: Option<StorageT>,
/// This rule's name. If None, then text which matches this rule will be skipped (i.e. will not
/// create a lexeme).
Expand All @@ -70,7 +71,7 @@ pub struct Rule<StorageT> {
pub target_state: Option<(usize, StartStateOperation)>,
}

impl<StorageT> Rule<StorageT> {
impl<StorageT: PrimInt> Rule<StorageT> {
/// Create a new `Rule`. This interface is unstable and should only be used by code generated
/// by lrlex itself.
#[doc(hidden)]
Expand Down Expand Up @@ -125,6 +126,19 @@ impl<StorageT> Rule<StorageT> {
target_state,
})
}

/// Return this rule's token ID, if any.
///
/// If `Some`, this specifies the ID that lexemes resulting from this rule will have. If
/// `None`, then this rule specifies lexemes which should not appear in the user's input.
pub fn tok_id(&self) -> Option<StorageT> {
self.tok_id
Copy link
Collaborator

@ratmice ratmice Dec 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is possible I'm missing something obvious, but I'm wondering if there is a reason that this doesn't return a cfgrammar::TIdx or some such?

Copy link
Member Author

@ltratt ltratt Dec 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also wondered that, but it's currently storing a StorageT. I can't fully remember what I might expect this to be, to be honest...

Copy link
Collaborator

@ratmice ratmice Dec 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it is also exposed publicly as StorageT via the lrpar::Lexeme trait here. So it doesn't seem to be unprecedented.

Copy link
Member Author

@ltratt ltratt Dec 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better as TIdx? If so, we might as well make the API as unsucky as we can reasonably make it!

Copy link
Collaborator

@ratmice ratmice Dec 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking further into it, I don't think TIdx would actually the right thing it should correspond to. For example, this usage in token_name isn't referring to one of these tok_id values at all, but an index into a Grammar specific array.

So, I'm leaning towards it being okay as is, at the very least nothing is ringing any bells for it being any of the other opaque type wrappers, and it certainly doesn't appear that TIdx would be right.

Copy link
Member Author

@ltratt ltratt Dec 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent detective work! OK I feel adequately comfortable with this PR now then.

}

/// Return the original regular expression specified by the user for this [Rule].
pub fn re_str(&self) -> &str {
&self.re_str
}
}

/// Methods which all lexer definitions must implement.
Expand Down