Skip to content

Ast-classes#79

Merged
CFU9 merged 19 commits into
sp-uulm:masterfrom
CFU9:master
Jan 22, 2021
Merged

Ast-classes#79
CFU9 merged 19 commits into
sp-uulm:masterfrom
CFU9:master

Conversation

@CFU9
Copy link
Copy Markdown
Collaborator

@CFU9 CFU9 commented Jan 17, 2021 •
edited by Lythenas
Loading

Closes #8

@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 17, 2021 •
edited
Loading

Codecov Report

Merging #79 (982a3c2) into master (ba7e358) will increase coverage by 4.19%.
The diff coverage is 84.20%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #79      +/-   ##
==========================================
+ Coverage   52.42%   56.61%   +4.19%     
==========================================
  Files          32       33       +1     
  Lines        3800     4377     +577     
==========================================
+ Hits         1992     2478     +486     
- Misses       1808     1899      +91
Impacted Files Coverage Δ
include/tree_sitter/tree_sitter.hpp 86.66% <ø> (ø)
src/details/ast.cpp 84.20% <84.20%> (ø)
src/stdlib.cpp 80.18% <0.00%> (-0.77%) ⬇️
src/values.cpp 67.53% <0.00%> (+0.14%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ba7e358...982a3c2. Read the comment docs.

@tn4799
Copy link
Copy Markdown
Collaborator

tn4799 commented Jan 18, 2021

Do you know that you don't have to close and reopen an PR? Just commit to your branch and it will be automatically added to the PR if it is not closed.
And the title and the description could be edited as well here at github

@CFU9 CFU9 linked an issue Jan 20, 2021 that may be closed by this pull request
Grammar errors and short commings #70
Open
2 tasks
Lythenas
Lythenas requested changes Jan 21, 2021
View reviewed changes
Copy link
Copy Markdown
Collaborator

@Lythenas Lythenas left a comment

Choose a reason for hiding this comment

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

I think generally it looks pretty good.

Can you please run the format script (and you should probably install the commit hook to do that automatically).

It may be worth merging some of the nodes. E.g.

  • VariableDeclaration and LocalVariableDeclaration can be merged with a bool saying if it is local or not.
  • VariableDeclaration and VariableDeclarator can be merged which might make it easier to use.

Comment thread include/tree_sitter/tree_sitter.hpp Outdated
Comment thread src/core/tree_sitter_ast.cpp Outdated
@Lythenas
Copy link
Copy Markdown
Collaborator

Lythenas commented Jan 21, 2021

include/MiniLua/tree_sitter_ast.hpp should be moved to std/details/ because it does not need to be part of our public api. src/core/tree_sitter_ast.cpp should also be moved to src/details/ and should probably be just named ast.cpp.

Lythenas
Lythenas requested changes Jan 22, 2021
View reviewed changes
Comment thread src/details/ast.cpp Outdated
// BinaryOperation
BinaryOperation::BinaryOperation(ts::Node node) : bin_op(node) {
if (node.type_id() != ts::NODE_BINARY_OPERATION || node.child_count() != 3) {
assert(node.child_count() == 3);
Copy link
Copy Markdown
Collaborator

@Lythenas Lythenas Jan 22, 2021

Choose a reason for hiding this comment

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

the asserts should be after the if because if we pass in the wrong type of node the exception is more appropriate.

@CFU9 CFU9 merged commit 3b0cc4d into sp-uulm:master Jan 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Lythenas Lythenas Lythenas approved these changes

Assignees

@CFU9 CFU9

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Grammar errors and short commings Write "temporary AST" classes

3 participants

@CFU9 @tn4799 @Lythenas