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

perf(parser): remove the return value option #47

Merged

Conversation

apengn
Copy link
Contributor

@apengn apengn commented Oct 17, 2023

remove the return value option

Which issue does this PR solve? (这个 PR 解决了什么问题)

#46

Retionable for this PR Change (这个 PR 做了什么改变)

remove the return value option

@apengn apengn force-pushed the remove_parse_expression_return_opton branch 2 times, most recently from f603f4b to 3ef84f9 Compare October 17, 2023 03:56
Copy link
Collaborator

@fansehep fansehep left a comment

Choose a reason for hiding this comment

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

God job! I will merge it later! ❤️
cc @Tangruilin PTAL.

@@ -460,7 +460,7 @@ impl Parser {

loop {
self.next_token();
exprs.push(self.parse_expression(Precedence::Lowest)?);
exprs.push(Some(self.parse_expression(Precedence::Lowest)?));
Copy link
Collaborator

Choose a reason for hiding this comment

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

oh, cc @Tangruilin

pub struct InsertStmt {
pub table_name: String,
pub columns: Option<Vec<String>>,
pub values: Vec<Vec<Option<Expression>>>,
}

looks InsertStmt values should't be Option?

Copy link
Contributor Author

@apengn apengn Oct 17, 2023

Choose a reason for hiding this comment

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

I haven't hacked into this InsertStmt structure, although it can also remove the Option, if possible, I can change it

Copy link
Collaborator

Choose a reason for hiding this comment

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

I haven't hacked into this InsertStmt structure,although it can also remove theOption`, if possible, I can change it

Emmm, you can open an other issue to trace this question if you want to solve it. 😃

Copy link
Collaborator

Choose a reason for hiding this comment

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

I haven't hacked into this InsertStmt structure,although it can also remove theOption`, if possible, I can change it

Emmm, you can open an other issue to trace this question if you want to solve it. 😃

I just saw it out of the blue and questioned it.

Comment on lines 868 to 849
Some(expr) => expr,
None => {
return Err(Error::ParseErr(fmt_err!(
"ON Predicate expression is not valid!"
)));
}
})
Some(self.parse_expression(Precedence::Lowest)?)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should keep this error message here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok,Pushed

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should keep this error message here.

And in other place. you hide the error message. May should keep them too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

Pushed @fansehep

if !is_prefix_oper(&self.pre_token) {
return Err(Error::ParseErr(fmt_err!(
"No prefixOperatorFunc for: {:?}",
"No prefix Operator Func for: {}",
Copy link
Collaborator

Choose a reason for hiding this comment

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

great! 👍🏻

@apengn apengn force-pushed the remove_parse_expression_return_opton branch from 3ef84f9 to b9e20de Compare October 17, 2023 05:16
@apengn apengn force-pushed the remove_parse_expression_return_opton branch from b9e20de to bbdfca9 Compare October 17, 2023 12:56
Copy link
Collaborator

@fansehep fansehep left a comment

Choose a reason for hiding this comment

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

LGTM.

@fansehep
Copy link
Collaborator

Really thanks for your contribution! @apengn ❤️

@fansehep fansehep merged commit 3704dab into shaun-io:master Oct 18, 2023
1 check 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.

2 participants