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

Update Tree-Sitter C grammar #3661

Merged
merged 2 commits into from
Jul 23, 2023
Merged

Update Tree-Sitter C grammar #3661

merged 2 commits into from
Jul 23, 2023

Conversation

XVilka
Copy link
Member

@XVilka XVilka commented Jul 17, 2023

Your checklist for this pull request

  • I've read the guidelines for contributing to this repository
  • I made sure to follow the project's coding style
  • I've documented or updated the documentation of every function and struct this PR changes. If not so I've explained why.
  • I've added tests that prove my fix is effective or that my feature works (if possible)
  • I've updated the rizin book with the relevant information (if needed)

Detailed description

As Tree-Sitter itself and C grammar fixed some of the issues that affected Rizin, update them before the release

Partially addresses #3498 (parsing stage)
Partially addresses #2308 (parsing stage)
Fixes #1666
Fixes #1337 (only left is preprocessor, should be handled differently: #3661)

Test plan

CI is green

@XVilka
Copy link
Member Author

XVilka commented Jul 22, 2023

The only left unfixed for now is #2190 but this one is harder and better to do separately (likely not in this release either as it depends on the mainstream bug: tree-sitter/tree-sitter-c#126)

@XVilka XVilka merged commit ba84a92 into dev Jul 23, 2023
Copy link
Member

@Rot127 Rot127 left a comment

Choose a reason for hiding this comment

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

Nothing which was related to this specific change. But the length could be reduced in the future. Should reduce the difficulty changing the code here.

node_malformed_error(state, child, text, "struct field");
result = -1;
goto srnexit;
}
Copy link
Member

Choose a reason for hiding this comment

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

All those repeating tests could probably be moved to a separated function.
So result = -1 is moved to the failure label. And replace it with

if (!parse_YYY()) {
   goto srn_fail_exit;
}

.name = membname,
.type = rz_type_clone(membtpair->type),
.offset = 0, // FIXME
.size = 0, // FIXME
Copy link
Member

Choose a reason for hiding this comment

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

Maybe describe what needs to be fixed here.

.offset = 0, // FIXME
.size = 0, // FIXME
};
void *element = rz_vector_push(members, &memb); // returns null if no space available
Copy link
Member

Choose a reason for hiding this comment

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

Comment that it returns NULL on failure can be removed.

// AST looks like
// type: (primitive_type) declarator: (field_identifier)
// or type: (primitive_type) declarator: (...) declarator (...)
// Augment resulting type field with the data from parsed declarator
Copy link
Member

Choose a reason for hiding this comment

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

Maybe not in this PR as well. But moving each case into a separated function should increase readability greatly.

.name = membname,
.type = rz_type_clone(membtpair->type),
.offset = 0, // FIXME
.size = 0, // FIXME
Copy link
Member

Choose a reason for hiding this comment

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

What needs to be fixed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support types with multiple declarators Mainstream Tree-Sitter issues for Rizin use cases
2 participants