2

I'm currently trying to decide whether to "structify" a rather long parameter set:

void fooCopy1(std::string const& source, std::string const& destination, std::string const& filter, std::string const& temp);

to this:

struct FooCopyArgs {
    std::string source;
    std::string destination;
    std::string filter;
    std::string temp;
};
void fooCopy2(FooCopyArgs const& args);

As already answered in two other questions:

refactoring this could have several readability/maintainability advantages. For a bunch of these see the linked questions.

In general however I see one "big" problem with this approach in C++ specifically, and that is that the strings will always have to be copied before the call to this function, instead of using const& parameters.

This would be against C++ Core Guideline F.16 "pass cheaply-copied types by value and others by reference to const".

That is, non-cheap readonly parameters that would normally be passed by const ref, would need to be copied into the struct, which would be a general pessimization.

(Yes, the struct itself would be passed by const ref, but the struct data members would need to copied first.)

Example:

const string temp = ...;
const string filter = ...;
...
fooCopy2({"sourceItem", "targetItem", filter, temp});

For "sourceItem", that is a locally defined parameter value, it would not matter. However, for the passed down args filterand temp we would have an extraneous copy that could be avoided by the plain const& approach.

Disclaimer: Obviously, in 99% of cases the performance impact won't even be observable in the final application, but still it leaves a bad taste, esp. in the context of some such "fundamental" rule as F.16.

Question : Is there any clever way around this problem, that is:

  • have a safe struct as parameter type (const& members are not safe; extremely prone to dangling references)
  • avoid extraneous copy of non-cheap types
  • keep composability if severeal functions use this pattern

Appendix: Why using const& members is unsafe:

struct HasConstRef {
    std::string const& member;
};

void f(HasConstRef const& arg) {
    std::cout << arg.member << "\n";
}

HasConstRef arg_producer() {
    HasConstRef result = { "there be dragons" };
    return result; // UB
}

void f_call() {
    f(arg_producer()); // *boom*, no diagnostic required
}

While I totally agree with the current answer that a const-ref-membered struct can be used correctly, it is also incredibly easy to use incorrectly without any help from the compiler. I would rather not do this.

I find that "hard to use incorrectly" is quite a long shot from "impossible to use incorrectly". And "normal" const-ref parameters, just like normal data members are hard-to-use-incorrectly (as far as C++ goes). const& members on the other hand are easy-to-use-incorrectly. Others seem to disagree: See the answer below. Alternatives still welcome.

10
  • 2
    Why copying? The strings might be stored as references or pointers in the struct as well. References rather, they are not optional anyway. One might need an appropriate constructor for. Commented Feb 20, 2023 at 14:12
  • 1
    If you are only ever passing one of these objects down the call stack (into a function) then having reference members is safe Commented Feb 20, 2023 at 14:13
  • @NathanOliver - yeah I know that it can be safe, but the struct and the whole thing would be extremely brittle and error prone without any compiler help. Commented Feb 20, 2023 at 14:16
  • @MartinBa What makes you think it's 'extremely brittle and error prone'? It's clearly documented in the class that these are references / non-null ptrs. Commented Feb 20, 2023 at 14:24
  • 1
    @MartinBa Then just add a ctor to HasConstRef or use pointers. (Btw., "there be dragons" won't disappear when you exit arg_producer(), it exists till static destruction at minimum, but I can guess what you were thinking about - and even then, that's an issue of arg_producer). Commented Feb 20, 2023 at 14:53

2 Answers 2

5

Sooo why did you remove const&? The following compiles fine:

#include <string>
using string = std::string;
struct FooCopyArgs {
    std::string const& source;
    std::string const& destination;
    std::string const& filter;
    std::string const& temp;
};
void fooCopy2(FooCopyArgs const& args);
int main() {
    const string temp = "";
    const string filter = "";
    fooCopy2({"sourceItem", "targetItem", filter, temp});
}
Sign up to request clarification or add additional context in comments.

7 Comments

OP's response when I pointed this out: yeah I know that it can be safe, but the struct and the whole thing would be extremely brittle and error prone without any compiler help
As soon as someone puts your FooCopyArgsWithConstRef version on the stack as a local variable it will break down. Without any compiler warnings.
This is the solution; however, since references aren't making it AggregateType, sometimes it's preferred to use pointers (perhaps with a non_null<std::string*> annotation). That makes it easy to write {&source, &destination, &filter, &temp} if needed, therefore making this better. (Alternatively, you can add a ctor.)
FooCopyArgsWithConstRef version on the stack as a local variable I do not understand. Can you give an example? If you put FooCopyArgs as a local variable in main, there is no difference, {...} creates a temporary anyway, it will be the same, just the lifetime of the variable will be veery slightly different.
@MartinBa Why do you think it breaks down? It's valid as long as the original args are valid. The only thing you don't have is extending the lifetime of a temporary at call (read: struct construction) point, but meh, if you do that, you need to be aware to put it in a variable.
|
2

In this case, I would worry less about performance, and more about semantics. Items should be combined into a struct or class if they really belong together. In other words, you want your classes and structs to have high cohesion.

Maybe filter and temp should be members of a class, and fooCopy() should be a member function in the same class. Presumably, filter and temp do not change often, whereas source and destination do.

By the way, consider renaming temp to something more descriptive.

Comments

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.