Skip to content

Change the parameter types of ParamValues's methods to avoid expensive conversion #8612

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
kawadakk opened this issue Dec 21, 2023 · 0 comments · Fixed by #8613
Closed

Change the parameter types of ParamValues's methods to avoid expensive conversion #8612

kawadakk opened this issue Dec 21, 2023 · 0 comments · Fixed by #8613
Labels
bug Something isn't working

Comments

@kawadakk
Copy link
Contributor

kawadakk commented Dec 21, 2023

Describe the bug

The parameter types of ParamValues::{get_placeholders_with_values,verify} could be changed to avoid expensive conversion on the caller side.

impl ParamValues {
    pub fn verify(&self, expect: &Vec<DataType>) -> Result<()>;
}

Receiving &[DataType] instead of &Vec<DataType> will allow the callers to pass an arbitrary slice without cloning it to a temporary Vec first. Clippy has a lint for this: ptr_arg

impl ParamValues {
    pub fn get_placeholders_with_values(
        &self,
        id: &String,
        data_type: &Option<DataType>,
    ) -> Result<ScalarValue>;
}

&String -> &str: ptr_arg likewise.

Receiving Option<&DataType> instead of &Option<DataType> will allow the callers to avoid cloning when they only have &DataType at hand. If the callers have &Option<DataType>, they will have to call Option::as_ref, but the performance cost of this conversion is negligible. There's a work-in-progress Clippy lint for this: rust-lang/rust-clippy#11463

To Reproduce

No response

Expected behavior

No response

Additional context

No response

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant