llvm/include/llvm/LTO/legacy/LTOCodeGenerator.h:100: pointless copy of a vector ?

[dcb314]

Static analyser cppcheck says:

llvm/include/llvm/LTO/legacy/LTOCodeGenerator.h:100:42: performance: Function parameter 'MAttrs' should be passed by const reference. [passedByValue]

Source code is

void setAttrs(std::vector MAttrs) { Config.MAttrs = MAttrs; }

[llvmbot]

Hi!

This issue may be a good introductory issue for people new to working on LLVM. If you would like to work on this issue, your first steps are:

1. Check that no other contributor has already been assigned to this issue. If you believe that no one is actually working on it despite an assignment, ping the person. After one week without a response, the assignee may be changed.
2. In the comments of this issue, request for it to be assigned to you, or just create a pull request after following the steps below. Mention this issue in the description of the pull request.
3. Fix the issue locally.
4. Run the test suite locally. Remember that the subdirectories under test/ create fine-grained testing targets, so you can e.g. use make check-clang-ast to only run Clang's AST tests.
5. Create a Git commit.
6. Run git clang-format HEAD~1 to format your changes.
7. Open a pull request to the upstream repository on GitHub. Detailed instructions can be found in GitHub's documentation. Mention this issue in the description of the pull request.

If you have any further questions about this issue, don't hesitate to ask via a comment in the thread below.

[llvmbot]

@llvm/issue-subscribers-good-first-issue

Author: None (dcb314)

Static analyser cppcheck says: llvm/include/llvm/LTO/legacy/LTOCodeGenerator.h:100:42: performance: Function parameter 'MAttrs' should be passed by const reference. [passedByValue] Source code is void setAttrs(std::vector<std::string> MAttrs) { Config.MAttrs = MAttrs; }

[dcb314]

Function signature hasn't changed, so I fail to understand how the commit fixes the problem.

AFAIK, the code still pointlessly copies a vector of string and changing the body of the function doesn't affect the signature.

Suggest use of const reference in the function signature.

[SimplyDanny]

Due to the internal move, the vector is now only copied once instead of twice previously. That's what Cppcheck actually complains about in my understanding.

[dcb314]

May I refer the right honourable gentleman to the answer I gave earlier ?

To quote:

Suggest use of const reference in the function signature.

Then zero copies, not one or two, will result.

[SimplyDanny]

It's unclear to me why there's no copy. Up to the point when the setter is called, the target variable Config.MAttrs is empty. Means, calling the setter results in a memory allocation. The constant reference would save a move, but how can there be no copy?

[dcb314]

but how can there be no copy?

Perhaps I've been slightly less than clear. Either that or perhaps you are new to C++ ?

Suggest change function signature from

void setAttrs(std::vector std::string MAttrs) {

to

void setAttrs(const std::vector std::string & MAttrs) {

That's all that cppcheck was complaining about.

If you don't understand the difference between these two signatures, can I be so bold as to suggest some C++ training ?

[SimplyDanny]

No worries, I very well understand what Cppcheck suggests. What I don't get is how that's superior compared to what there's now. Cppcheck, at least, appears to be happy with the "fix". But never mind, seems like I'm not going to find an answer here after reflecting and asking three times.

Can I be so bold as to suggest you some communication training?

[dcb314]

What I don't get is how that's superior compared to what there's now.

Then you don't understand a basic feature of C++ parameter passing. More detail on

https://www.learncpp.com/cpp-tutorial/pass-by-const-lvalue-reference/

Can I be so bold as to suggest you some communication training?

Always a wise idea. I've been doing C++ for nearly 40 years now and sometimes forget we were all new once.

Thanks for your persistence.