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

allow identifiers in string concatentations #145

Merged
merged 3 commits into from
Jul 15, 2023
Merged

allow identifiers in string concatentations #145

merged 3 commits into from
Jul 15, 2023

Conversation

clason
Copy link
Contributor

@clason clason commented Jul 15, 2023

Resolves errors in strings of the form:

("failed in line %" PRIdLINENR)

@clason
Copy link
Contributor Author

clason commented Jul 15, 2023

cherry-picked from neovim#2 @lewis6991 @amaanq

@amaanq amaanq requested a review from maxbrunsfeld July 15, 2023 09:38
@amaanq
Copy link
Member

amaanq commented Jul 15, 2023

@ahlinc would like your thoughts too

Resolves errors in strings of the form:

   ("failed in line %" PRIdLINENR)
@clason clason changed the title allow identifiers in string concatentations (#2) allow identifiers in string concatentations Jul 15, 2023
@ahlinc
Copy link

ahlinc commented Jul 15, 2023

Screenshot from 2023-07-15 14-40-02

@ahlinc would like your thoughts too

I'd also alias the identifier node to the same node that represent a string content between double quotes but unfortunately there is no one.

It make sense to add a bellow additional fix on top of this PR:

diff --git i/grammar.js w/grammar.js
index 94e7f3d..b51cacd 100644
--- i/grammar.js
+++ w/grammar.js
@@ -1055,13 +1055,16 @@ module.exports = grammar({

     concatenated_string: $ => seq(
       $.string_literal,
-      repeat1(choice($.string_literal, $.identifier)),
+      repeat1(choice(
+        $.string_literal,
+        alias($.identifier, $.string),
+      )),
     ),

     string_literal: $ => seq(
       choice('L"', 'u"', 'U"', 'u8"', '"'),
       repeat(choice(
-        token.immediate(prec(1, /[^\\"\n]+/)),
+        alias(token.immediate(prec(1, /[^\\"\n]+/)), $.string),
         $.escape_sequence,
       )),
       '"',

@amaanq
Copy link
Member

amaanq commented Jul 15, 2023

Maybe string_content is better than string, but I don't think identifiers should be aliased to string in concatenations

@amaanq
Copy link
Member

amaanq commented Jul 15, 2023

Also gotta get rid of the appveyor thing after this

@clason tests need updates

@clason
Copy link
Contributor Author

clason commented Jul 15, 2023

Used tree-sitter test -u; hope that's OK!

@amaanq
Copy link
Member

amaanq commented Jul 15, 2023

Mentioned this earlier here, but the formatting of the header/test delimiters needs to not happen, it pollutes the diff (not your problem, it looks fine to me 😅)

@amaanq amaanq merged commit 6adee19 into tree-sitter:master Jul 15, 2023
@clason
Copy link
Contributor Author

clason commented Jul 15, 2023

Would it be acceptable for me to do a separate commit before the change to reformat the corpus? I think the convenience of using tree-sitter test -u is worth a single reformat churn.

@clason clason deleted the string-concat branch July 15, 2023 12:45
@amaanq
Copy link
Member

amaanq commented Jul 15, 2023

hmm not sure what you mean, like a commit of additional tests?

@clason
Copy link
Contributor Author

clason commented Jul 15, 2023

like a commit just calling tree-sitter test -u on the old parser to reformat everything (without additional changes), so that the next commit with the changes can use tree-sitter test -u to only update the now failing tests, without polluting the diff.

@amaanq
Copy link
Member

amaanq commented Jul 15, 2023

Oooh, yeah I'm way too tired my bad for not getting that - that would be nicer for the future

@ahlinc
Copy link

ahlinc commented Jul 15, 2023

but I don't think identifiers should be aliased to string in concatenations

@amaanq in concatenations identifiers aren't identifiers because they are concatenated literally and not by value.

@amaanq
Copy link
Member

amaanq commented Jul 15, 2023

well it's like an identifier that holds a string, so it shouldn't be aliased as such so users can distinguish them imo

@ahlinc
Copy link

ahlinc commented Jul 15, 2023

well it's like an identifier that holds a string, so it shouldn't be aliased as such so users can distinguish them imo

sorry, my bad, bash in my mind.

For C the PRIdLINENR would be an identifier that would be substituted by a macro pre proccessor to an another string.

@ahlinc
Copy link

ahlinc commented Jul 15, 2023

Hmm, for the clarity may be it would still make sense to alias identifier to the preproc_arg for example, because it's not real C variable and in this place it's clear how it can appear in concatenation.

Screenshot from 2023-07-15 23-16-19

😢 Preproc directives parsing definitely requires improvements.

@amaanq
Copy link
Member

amaanq commented Jul 15, 2023

Preproc directives parsing definitely requires improvements.

Agreed..

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.

4 participants