feat(minifier): implement folding indexOf and lastIndexOf string fns

[camc314]

No description provided.

[graphite-app]

Your org has enabled the Graphite merge queue for merging into main

Add the label “0-merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

[camc314]

• feat(minifier): implement folding charCodeAt string fns #6475
• feat(minifier): implement folding charAt string fns #6436
• feat(minifier): implement folding indexOf and lastIndexOf string fns #6435 👈
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @camc314 and the rest of your teammates on Graphite

[codspeed-hq]

CodSpeed Performance Report

Merging #6435 will not alter performance

_{Comparing c/10-10-feat_minifier_implement_folding_indexof_and_lastindexof_string_fns (a4f57a4) with main (5ee1ef3)}

Summary

✅ 29 untouched benchmarks

[7086cmd]

I propose that we implement these functionalities using the &string type, as per the ECMA specification (with some std syntax candies in Rust). This approach is more convenient.

Once the target string is literal, meaning that we can use ctx.get_string_value with a side effect, we can apply the functionality and implement it.

Here is a snippest:

    fn string_index_of(string: &'a str, search_value: &'a str, from_index: usize) -> Option<usize> {
        let len = string.len();
        if from_index > len {
            return None;
        }
        if search_value.is_empty() {
            return Some(from_index);
        }
        string[from_index..].find(search_value).and_then(|pos| Some(pos + from_index))
    }

Then we can visit the Expression which is CallExpression, whose callee is a string literal, and handle these methods in a same position.

[7086cmd]

I propose that we implement these functionalities using the &string type, as per the ECMA specification (with some std syntax candies in Rust). This approach is more convenient.

Btw that's why I put the file in a folder with mod.rs.

[camc314]

I propose that we implement these functionalities using the &string type, as per the ECMA specification (with some std syntax candies in Rust). This approach is more convenient.

Once the target string is literal, meaning that we can use ctx.get_string_value with a side effect, we can apply the functionality and implement it.

Here is a snippest:

    fn string_index_of(string: &'a str, search_value: &'a str, from_index: usize) -> Option<usize> {
        let len = string.len();
        if from_index > len {
            return None;
        }
        if search_value.is_empty() {
            return Some(from_index);
        }
        string[from_index..].find(search_value).and_then(|pos| Some(pos + from_index))
    }

Then we can visit the Expression which is CallExpression, whose callee is a string literal, and handle these methods in a same position.

i see this is a good idea, let me refactor this tomorrow - thanks!

[camc314]

@7086cmd is this more what you had in mind?

[7086cmd]

Is this more what you had in mind?

Yes exactly! Also you can move them into a trait so that you can remove these lifetime tags.

[camc314]

are you thinking like this?

trait StringSearch {
    fn evaluate_string_index_of(
        &self,
        search_value: Option<&str>,
        from_index: Option<f64>,
    ) -> isize;
}

impl StringSearch for str {
    fn evaluate_string_index_of(
        &self,
        search_value: Option<&str>,
        from_index: Option<f64>,
    ) -> isize {
        let from_index = from_index.map_or(0, |x| x.to_js_int_32().max(0)) as usize;
        let Some(search_value) = search_value else {
            return -1;
        };
        let result = self.chars().skip(from_index).collect::<String>().find(search_value);
        result.map(|index| index + from_index).map_or(-1, |index| index as isize)
    }
}

or something else? feel free to just point me at an example 🙂

[7086cmd]

or something else? feel free to just point me at an example 🙂

Good, and I recommend you use different structs to handle different types. That is to say, you can implement indexOf, substr, etc. in one struct for strings, and you can use the name StringUtils. Also, Math.sin, Math.floor, etc. for MathUtils, etc.

[camc314]

or something else? feel free to just point me at an example 🙂

Good, and I recommend you use different structs to handle different types. That is to say, you can implement indexOf, substr, etc. in one struct for strings, and you can use the name StringUtils. Also, Math.sin, Math.floor, etc. for MathUtils, etc.

i don't think this avoids the lifetimes

or something else? feel free to just point me at an example 🙂

Good, and I recommend you use different structs to handle different types. That is to say, you can implement indexOf, substr, etc. in one struct for strings, and you can use the name StringUtils. Also, Math.sin, Math.floor, etc. for MathUtils, etc.

cool makes sense - thanks!

[7086cmd]

Nice! I may create a commit to comment these match branches and mark TODO, as these are not necessary to appear in effective code or (maybe) raise ambiguity and reduce readability.

[Boshen]

Merge activity

• Oct 12, 12:32 AM EDT: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
• Oct 12, 12:32 AM EDT: A user added this pull request to the Graphite merge queue.
• Oct 12, 12:41 AM EDT: A user merged this pull request with the Graphite merge queue.