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

add random_replacetext, a replacement for stars() #168

Closed
wants to merge 6 commits into from

Conversation

Kapu1178
Copy link
Contributor

Adds a new function, random_replacetext(), which randomly replaces non-whitespace characters in a string with another character. This is to replace the stars() function found in every SS13 codebase, which is awfully, awfully slow due to byond's string interning.

Please ignore the overtime values for this, as I simply ran each proc 10,000 times in the same tick.
image

dmsrc/random_replacetext.dm Outdated Show resolved Hide resolved
Copy link
Collaborator

@ZeWaka ZeWaka left a comment

Choose a reason for hiding this comment

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

i don't think this is worth the compile time of 2 seconds max, much less the maintainer overhead and me having to write this sentence

@Kapu1178
Copy link
Contributor Author

i don't think this is worth the compile time of 2 seconds max, much less the maintainer overhead and me having to write this sentence

So, let's put into perspective why this is useful. This proc, on TG, is labelled as so expensive you should never use it. It used to be a crash vector on Paradise. This proc used to be used alot, but was scrubbed away because of the cost it incurred. Besides, this code is braindead simple (because im braindead and I wrote it), maintaining it isn't exactly a challenge.

Copy link
Collaborator

@ZeWaka ZeWaka left a comment

Choose a reason for hiding this comment

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

blocking: feature not documented properly in code nor in readme

if you can get another maintainer not from daedalus to care about this we can merge it

});

fn replacetext(text: &str, prob_as_str: &str, replacement_str: &str) -> Result<String> {
let prob = prob_as_str.parse::<u32>()?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Have you considered how this error will propagate up?

@Kapu1178 Kapu1178 closed this Apr 21, 2024
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.

2 participants