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

feat: rewrite the scanner in C #29

Merged
merged 2 commits into from
Feb 23, 2024
Merged

Conversation

amaanq
Copy link
Contributor

@amaanq amaanq commented Jun 21, 2023

No description provided.

@vkleen
Copy link
Contributor

vkleen commented Jun 22, 2023

Do you have a particular rationale for rewriting the scanner in C instead of C++?

@amaanq
Copy link
Contributor Author

amaanq commented Jun 23, 2023

Portability and ease of compilation - it better supports projects using a pure C toolchain and the logic is simple enough to not really need the C++ containers

Copy link
Contributor

@yannham yannham left a comment

Choose a reason for hiding this comment

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

Hi, and thanks for the contribution!

IMHO inlining the implementation of vector using a custom struct and C macros isn't a reasonable tradeoff. If @vkleen or anyone from the Topiary team reports that there is strong evidence that this fixes their issues for compiling Topiary as a static binary for example, or that it might solve someone's actual and very concrete issue, then it might be considered. Until then I would personally not endorse this change based on a "this might probably help with", considering it doesn't have a null cost and that it doesn't fix any known bug directly (put differently, "if it ain't broke, don't fix it")

@amaanq
Copy link
Contributor Author

amaanq commented Jun 26, 2023

Sure, those are valid concerns - but I've been rewriting many C++ scanners across various tree-sitter repos - for the most part, it doesn't solve a particular bug and it really is just about making compilation easier and improving portability. Though in some cases, there were bugs with leaks or buffer overflows in the original scanner that I had fixed anyways (not this one)

@amaanq
Copy link
Contributor Author

amaanq commented Jul 27, 2023

I'll also tack on that building this for wasm is much easier than with C++ due to the need to manually specify specific std functions to export (in tree-sitter core there is a file that manually picks common functions to export)

Anyways I updated it to add static to any helper functions

@clason
Copy link

clason commented Feb 22, 2024

I'll also add that upstream has officially deprecated C++ scanners, and such parsers will be removed in nvim-treesitter 1.0.

@yannham
Copy link
Contributor

yannham commented Feb 23, 2024

Thanks for the information, @clason. I think this is a good argument in favor of moving the scanner to C, despite the loss of some nice abstractions.

@clason
Copy link

clason commented Feb 23, 2024

Upstream is adding some nice abstractions to the C headers, though, so it might not be too bad: tree-sitter/tree-sitter#3063

Copy link
Contributor

@yannham yannham left a comment

Choose a reason for hiding this comment

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

Thanks again for undertaking that. We'll probably need to rebase and regenerate the parser, as there's a conflict currently.

src/scanner.c Outdated Show resolved Hide resolved
src/scanner.c Outdated Show resolved Hide resolved
@amaanq
Copy link
Contributor Author

amaanq commented Feb 23, 2024

Thanks again for undertaking that. We'll probably need to rebase and regenerate the parser, as there's a conflict currently.

C++ is a pain to get working cross-platform and in wasm especially, and there's really no need for most of its abstractions. The only used abstractions across grammars with a scanner is vectors/strings, which the near-future inclusion of an array header will provide a decent replacement for.

@amaanq
Copy link
Contributor Author

amaanq commented Feb 23, 2024

I've rebased now @yannham

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.

None yet

5 participants