[clang-tidy] Check request: readability-pass-std-string-view-by-value

[denzor200]

Since std::string_view is a lightweight object with reference semantic, there is no objective reason to pass it by reference.

void process_1(const std::string_view&) {} // INCORRECT
void process_2(std::string_view&&) {}      // INCORRECT
void process_3(std::string_view) {}        // OK

The exception is when it was passed as an in-out parameter.

void process_and_change(std::string_view& sv_inout) {
    for (char c : sv_inout) {
        // a logic to process input characters...
    }
    sv_inout = get_new_token();
}

[llvmbot]

@llvm/issue-subscribers-clang-tidy

Author: Denis Mikhailov (denzor200)

Since std::string_view is a lightweight object with reference semantic, there is no objective reason to pass it by reference.

void process_1(const std::string_view&) {} // INCORRECT
void process_2(std::string_view&&) {}      // INCORRECT
void process_3(std::string_view) {}        // OK

The exception is when it was passed as an in-out parameter.

void process_and_change(std::string_view& sv_inout) {
    for (char c : sv_inout) {
        // a logic to process input characters...
    }
    sv_inout = get_new_token();
}

[safocl]

void process_and_change(std::string_view& sv_inout)

maybe it's better to return a new object (std::string or std::string_view)?

[PiotrZSL]

This should be part of an modernize-pass-by-value

[ArnaudBienner]

I agree there is no reason to pass it by reference, however does it really hurt?

[safocl]

I agree there is no reason to pass it by reference, however does it really hurt?

this is additional indirection... (such as TYPE** )

[carlosgalvezp]

Do you have an example where the assembly code with optimizations (O2) is worse when passing string-view by value vs by reference?

[safocl]

Do you have an example where the assembly code with optimizations (O2) is worse when passing string-view by value vs by reference?

such as this?

mov rax, qword ptr [rdi + 8]
vs
mov rax, rsi

[carlosgalvezp]

Thanks, there is indeed a difference in the assembly. The real question is, does in matter in practice? Micro-optimizations must be done having profiling data to back up the need for such optimization. "Premature optimization is the root of all evil".

I ran your example through QuickBench and I was not able to see any difference, but maybe I'm doing something wrong - good if you can double-check!

[safocl]

Thanks, there is indeed a difference in the assembly. The real question is, does in matter in practice? Micro-optimizations must be done having profiling data to back up the need for such optimization. "Premature optimization is the root of all evil".

I ran your example through QuickBench and I was not able to see any difference, but maybe I'm doing something wrong - good if you can double-check!

if you check it on a real example in any significant project, it seems that each unnecessary dereference will have a significant loss of productivity in total...

[carlosgalvezp]

Like I said, this is something one should measure, not speculate it will be "significant" :)

But anyway, I think there are other use cases for a general-purpose performance-pass-by-value check:

• built-in types (ints, floats, pointers, etc)
• Selected classes (std::stringview, std::span, etc). This should be user-configurable.
• Anything else?

[denzor200]

I suppose all what we really need is a check that implements F.16 from the C++ Core Guidelines.
Look at one of it's enforcement: "Warn when a parameter passed by reference to const has a size less or equal than 2 * sizeof(void*). Suggest passing by value instead."

[carlosgalvezp]

I think that's actually very poor advice. User-defined classes can change in size over their lifetime. Today, they can have 16 bytes; tomorrow, a field can get added and go over that threshold. Now, a silly change forces you to change the entire codebase to pass by const& instead of by value. And viceversa when you remove a field.

Instead, one should only warn for classes that are not expected to change in size, not "any" class blindly.

[denzor200]

User-defined classes can change in size over their lifetime. Today, they can have 16 bytes; tomorrow, a field can get added and go over that threshold. Now, a silly change forces you to change the entire codebase to pass by const& instead of by value. And viceversa when you remove a field.

Nice point indeed! But this issue is on the side of the C++ Core Guidelines