Public functions for Rule's internals
#478
Conversation
These are properties that users sometimes want to access, but it's better if we don't directly expose the underlying fields.
This will allow us to, eventually, de-`pub` it.
| /// 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
Looks like it is also exposed publicly as StorageT via the lrpar::Lexeme trait here. So it doesn't seem to be unprecedented.
There was a problem hiding this comment.
Would it be better as TIdx? If so, we might as well make the API as unsucky as we can reasonably make it!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Excellent detective work! OK I feel adequately comfortable with this PR now then.
|
Besides that one question I had (which it seems we can disregard) it looked good to me. |
2 participants
This PR first fixes #477 and then goes further and gives the other currently-
pubfields accessor functions, and marks the fields as deprecated. This will allow us -- one day! -- to removepubfrom the fields. Right now, this change doesn't require a major release.