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.

Sign up for GitHub

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

Jump to bottom

Add \ref support for strings #720

Merged
merged 13 commits into from
Sep 29, 2022
Merged

Add \ref support for strings #720

merged 13 commits into from
Sep 29, 2022

Conversation

ike709
Copy link
Collaborator

@ike709 ike709 commented Jun 12, 2022

No description provided.

@Hinaichigo
Copy link
Contributor

Is this related to #700?

@ike709
Copy link
Collaborator Author

ike709 commented Jun 12, 2022

no

wixoaGit
wixoaGit requested changes Jun 13, 2022
View reviewed changes
Copy link
Member

@wixoaGit wixoaGit left a comment

Choose a reason for hiding this comment

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

There's no way for locate() to know when the reference is for a string. Currently it will try to find an object with the string's ID.

@ike709 ike709 marked this pull request as draft June 14, 2022 17:56
@ike709 ike709 marked this pull request as ready for review June 14, 2022 18:09
@ike709 ike709 requested a review from wixoaGit June 14, 2022 18:09
@ike709
Copy link
Collaborator Author

ike709 commented Jun 14, 2022

Spurious CI failure again. I think github is having an issue.

@ike709 ike709 mentioned this pull request Jun 15, 2022
Closed
15 tasks
@ike709 ike709 marked this pull request as draft July 16, 2022 21:05
@ike709 ike709 marked this pull request as ready for review July 18, 2022 04:10
@ike709 ike709 requested a review from wixoaGit July 18, 2022 04:10
@PJB3005
Copy link
Contributor

PJB3005 commented Jul 18, 2022

Isn't this going to permanently leak all strings passed through \ref? You may want a different string tree that uses weak references instead, and some sort of cleanup routine.

@ike709
Copy link
Collaborator Author

ike709 commented Jul 18, 2022

True but this is still pretty much a placeholder ref implementation anyways to bandaid round init while we work on other things.

wixoaGit
wixoaGit requested changes Sep 16, 2022
View reviewed changes
@ike709 ike709 marked this pull request as draft September 16, 2022 19:29
@ike709 ike709 requested a review from wixoaGit September 24, 2022 21:43
@ike709 ike709 marked this pull request as ready for review September 24, 2022 21:43
@ike709
Copy link
Collaborator Author

ike709 commented Sep 24, 2022

Review addressed but I haven't thoroughly tested the changes.

@wixoaGit
Copy link
Member

This breaks locate() for DreamObjects currently. locate("10") looks for a DreamObject with ID 10 instead of 0.

@ike709 ike709 marked this pull request as draft September 26, 2022 20:27
@ike709 ike709 marked this pull request as ready for review September 27, 2022 23:30
@ike709
Copy link
Collaborator Author

ike709 commented Sep 27, 2022

Ready for review

@wixoaGit
Copy link
Member

Isn't this going to permanently leak all strings passed through \ref? You may want a different string tree that uses weak references instead, and some sort of cleanup routine.

Yes, this will allow you to get any string existing at compile time or passed through ref(). This is actually similar behavior to byond, which can be observed with the following code:

for (var/i in 1 to 0x1a0)
	world.log << locate("\[0x6000[num2text(i, 3, 16)]\]")

I don't think a cleanup routine would be possible, as there's no way to tell when the code will want to locate() it.

wixoaGit
wixoaGit requested changes Sep 29, 2022
View reviewed changes
@wixoaGit wixoaGit enabled auto-merge (squash) September 29, 2022 16:22
@wixoaGit wixoaGit merged commit d222045 into OpenDreamProject:master Sep 29, 2022
Altoids1 added a commit to Altoids1/OpenDream that referenced this pull request Oct 1, 2022
@Altoids1
Re-implements new behaviour from OpenDreamProject#801, OpenDreamProje…
e8faaeb 
…ct#720

I moved this code over into a helper proc so this new stuff has to be re-added here, awkwardly.

EDIT: Also patched for some changes caused by refs becoming semi-implemented.
wixoaGit pushed a commit that referenced this pull request Oct 11, 2022
@Altoids1
Fixes several bugs with string macros, implements all pronoun/article…
3a06b8b 
… ones (#777)

* Moves String Token parsing into a helper proc

DMParser.cs is >2300 lines long before this change, and implementing pronouns is going to make it even longer, so I figure its time to put the helper file to some use.

Marked as a separate commit so you have better diffs to look at during review :^)

* Parser now ensures that string macros have interp value if needed

A dangling \hers or \The now causes a parsing error, as it should.

* Improves quality of string macro errors/warnings

We now, like BYOND, give a descriptive error message when user tries to use \her (a macro that can't exist).

While I was here, I decided to be even sweeter and add some leniency with unnecessarily-capitalized macros. 😊

* Refactors StringFormatType into a static class thing

Storing all of our formatting bytes as 0x00FF 0x00-whatever was both breaking parity and wasting bytes, so that's been completely changed in favour of a static class that handles encoding and decoding from formatting bytes, stored passive-aggressively into a not-as-important Unicode codeblock. Various other changes were made to prepare for a wider implementation of text macros.

This compiled and ran OK, so I'm committing here before I start implementing novel functionality.

* Adds null opcode checking in InternalResume()

I had a really strange bug recently and this helped me with debugging, so might as well commit it to master.

* Implements \the, \The, \A, \An

This gets the general pattern going for formatted strings.

At first, I thought that I was going to have to do two passes on the string to actually parse it correctly (to harvest the locations of all the interp values, so I know who owns what macro). Realized that I could just push another operand to the opcode proc and save a lot of that time, so that's what's here.

* Adds unimplemented warnings for \s and \th

Previously, they would actually cause a compiletime error were they to appear. Now the compiler treats them as it should.

* Implements suffix pronouns (\he, \himself, etc)

I need a better place to toss all these helpers.
EDIT: Accidentally left debug logging in there, whoops

* Re-implements new behaviour from #801, #720

I moved this code over into a helper proc so this new stuff has to be re-added here, awkwardly.

EDIT: Also patched for some changes caused by refs becoming semi-implemented.

* Adds some new unit tests for string interpolation

EDIT: adds StringInterpolation4.dm
EDIT EDIT: and the fifth test too

* Fixes weird bug with reversed string interp values

* Fixes whitespace not making strings proper

* Fixes \ref[] interpolations being used by suffix pronouns

* Wixoa review commit
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wixoaGit wixoaGit wixoaGit requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ike709 @Hinaichigo @PJB3005 @wixoaGit