-
Notifications
You must be signed in to change notification settings - Fork 107
Use reference counted scheme instead of references #119
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
Conversation
2215ca3 to
ae4472e
Compare
19a2faa to
b269877
Compare
| let (function, rest) = Function::lex_with(input, parser.scheme)?; | ||
| impl<'i> LexWith<'i, &FilterParser<'_>> for FunctionCallExpr { | ||
| fn lex_with(input: &'i str, parser: &FilterParser<'_>) -> LexResult<'i, Self> { | ||
| let (function, rest) = FunctionRef::lex_with(input, parser.scheme)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar here, should lex_with be implemented on Function, so we don't need to call .to_owned() on the FunctionRef?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a strong opinion. It's just going to move the call to .to_owned() to the caller so its not clear what we would gain.
|
|
||
| #[inline] | ||
| pub(crate) fn get_field_value_unchecked(&self, field: Field<'_>) -> &LhsValue<'_> { | ||
| pub(crate) fn get_field_value_unchecked(&self, field: &Field) -> &LhsValue<'_> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this take a FieldRef instead of a &Field (and similarly for get_list_matcher_unchecked below)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so, since this is used specifically to get the value from a Field stored in the ast, it seems wasteful to have to convert to a FieldRef before.
engine/src/scheme.rs
Outdated
| /// An enum to represent an entry inside a [`Scheme`](struct@Scheme). | ||
| /// It can be either a [`Field`](struct@Field) or a [`Function`](struct@Function). | ||
| #[derive(Debug)] | ||
| pub enum Identifier<'s> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be called IdentifierRef now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its not exposed anymore since #122 so I am not sure it matters much.
I actually wonder if we should just get rid of that type entirely.
83387ac to
564268d
Compare
This allows to remove the `'s` lifetime in most of the codebase which should also simplify the life of wirefilter users. It also enables downstream crates to implement deserialization of filter AST with serde using TLS stored scheme or similar tricks.
564268d to
fd35321
Compare
This allows to remove the
'slifetime in most of the codebase which should also simplify the life of wirefilter users.It also enables downstream crates to implement deserialization of filter AST with serde using TLS stored scheme or similar tricks.