Skip to content

Conversation

@timkaechele
Copy link
Contributor

@timkaechele timkaechele commented Oct 18, 2025

This PR starts using the hb_string_T in the interface of herb_analyze.

This is a side effect free refactor, that makes the switch to hb_string_T based token values easier later on.

@timkaechele timkaechele force-pushed the use-string-in-analyze-ruby-method branch from f2f848e to 49dce47 Compare October 18, 2025 10:49
@timkaechele timkaechele marked this pull request as ready for review October 18, 2025 10:49
@timkaechele timkaechele changed the title C: Use hb_string in herb_analyze function C: Use hb_string_T in herb_analyze function Oct 18, 2025

if (strcmp(opening, "<%%") != 0 && strcmp(opening, "<%%=") != 0 && strcmp(opening, "<%#") != 0) {
analyzed_ruby_T* analyzed = herb_analyze_ruby(erb_content_node->content->value);
analyzed_ruby_T* analyzed = herb_analyze_ruby(hb_string_from_c_string(erb_content_node->content->value));
Copy link
Owner

@marcoroth marcoroth Oct 18, 2025

Choose a reason for hiding this comment

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

I wonder, if we eventually should change the StringField in template.rb to just be a hb_string_T on all AST Nodes, so we don't have to always convert them when using them.

Copy link
Contributor Author

@timkaechele timkaechele Oct 18, 2025

Choose a reason for hiding this comment

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

Long term that's definitely the plan.

I just try to get there in small steps so that we won't have a giant PR that is hard to test, ideally we can always easily check for regressions.

Copy link
Owner

@marcoroth marcoroth left a comment

Choose a reason for hiding this comment

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

Thanks @timkaechele! 🙏🏼

@marcoroth marcoroth merged commit 2b4074e into marcoroth:main Oct 18, 2025
13 checks passed
@timkaechele timkaechele deleted the use-string-in-analyze-ruby-method branch October 18, 2025 15:26
asilano pushed a commit to fac/herb that referenced this pull request Oct 21, 2025
This PR starts using the `hb_string_T` in the interface of
`herb_analyze`.

This is a side effect free refactor, that makes the switch to
`hb_string_T` based token values easier later on.
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.

2 participants