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

Specially handle short strings in the parser #475

Merged
merged 2 commits into from
Oct 22, 2024
Merged

Conversation

pschachte
Copy link
Owner

These short strings are read using the appropriate constructor, rather than the general buffer
constructor. So the empty and singleton
constructors are now public.

Other changes include optimising foreign lpvm cast instructions applied to constant chars, integers
and floats, as they get generated in this case.
This avoids several instructions in calling
unboxed constructors with constant arguments.

Also use the -n wybemk flag in final-dump-test.sh, to turn off colourising, making diffs easier to
read.

There are numerous code improvements in final dump tests, including allowing specialisation to kick
in in many places, since unboxed values cannot be
aliased.

There's one unfortunate change in output for
constant_type_constraint_error.wybe: the error
message for ?a = "s":int now complains about a
type error in the call to wybe.string.singleton,
which doesn't appear on the line. I think this is
tolerable for now, as it would be difficult to
fix.

Closes #473

These short strings are read using the appropriate
constructor, rather than the general buffer
constructor.  So the empty and singleton
constructors are now public.

Other changes include optimising foreign lpvm cast
instructions applied to constant chars, integers
and floats, as they get generated in this case.
This avoids several instructions in calling
unboxed constructors with constant arguments.

Also use the -n wybemk flag in final-dump-test.sh,
to turn off colourising, making diffs easier to
read.

There are numerous code improvements in final dump
tests, including allowing specialisation to kick
in in many places, since unboxed values cannot be
aliased.

There's one unfortunate change in output for
constant_type_constraint_error.wybe: the error
message for ?a = "s":int now complains about a
type error in the call to wybe.string.singleton,
which doesn't appear on the line. I think this is
tolerable for now, as it would be difficult to
fix.

Closes #473
@pschachte pschachte requested a review from jimbxb October 21, 2024 10:24
src/Parser.hs Outdated Show resolved Hide resolved
src/BodyBuilder.hs Outdated Show resolved Hide resolved
src/BodyBuilder.hs Show resolved Hide resolved
wybelibs/wybe/string.wybe Show resolved Hide resolved
@pschachte pschachte merged commit af2f3b3 into master Oct 22, 2024
1 check passed
@pschachte pschachte deleted the special_strings branch October 22, 2024 05:37
@jimbxb
Copy link
Collaborator

jimbxb commented Oct 22, 2024

Sorry for being a bit late to the party, but I'm not sure Expansion is the right place to do this. I believe this string specialisation pass should only ever happen once, as the transformation is idempotent. Expansion may only get ran once at the moment (correct me if I am wrong), but theoretically it could get ran multiple times, so the specialisation pass would be redundant after the first Expansion pass in the module. I realise that we haven't been focusing on making the compiler the most efficient, but I feel we should avoid inefficiencies if we are aware of them.

@pschachte
Copy link
Owner Author

Expansion is only run once (not positive there are no circumstances when it is repeated, but I think it's not). But you're right, we may want to repeat it. This shouldn't be a problem, though, since after this is done, we don't have the ArgString constructor for empty or singleton strings, so the change is idempotent. We're already scanning every argument during expansion, to update variables, so looking at ArgStrings and checking them for empty or singleton lists is a minimal added overhead. And if we moved it somewhere else, it would impose a similar overhead.

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.

Empty and singleton string constants should generate empty and singleton(c) instead of buffer(...)
2 participants