1

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.

Martin Ba
  • 37,187
  • 33
  • 183
  • 337
  • 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. – Aconcagua Feb 20 '23 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 – NathanOliver Feb 20 '23 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. – Martin Ba Feb 20 '23 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. – lorro Feb 20 '23 at 14:24
  • @MartinBa Can you explain what exactly do you mean by "extremely brittle and error prone"? What compiler help do you have in mind? – KamilCuk Feb 20 '23 at 14:25
  • @KamilCuk - have provided example of why I find the const-ref-member solution too brittle. – Martin Ba Feb 20 '23 at 14:44
  • "Appendix: Why using const& members is unsafe:" the only "safe" solution to the general issue to not use C++. Either you accept that c++ is not a safe language or not – 463035818_is_not_an_ai Feb 20 '23 at 14:50
  • 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`). – lorro Feb 20 '23 at 14:53
  • I second lorro, you can always write a function the returns a dangling pointer / reference for any type. Thats not the fault of `HasConstRef` – 463035818_is_not_an_ai Feb 20 '23 at 14:54
  • This use case is safe, not brittle, and not error prone. – Eljay Feb 20 '23 at 17:20

2 Answers2

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});
}
KamilCuk
  • 120,984
  • 8
  • 59
  • 111
  • 1
    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* – NathanOliver Feb 20 '23 at 14:24
  • As soon as someone puts your `FooCopyArgsWithConstRef` version on the stack as a local variable it will break down. Without any compiler warnings. – Martin Ba Feb 20 '23 at 14:26
  • This _is_ the solution; however, since references aren't making it AggregateType, sometimes it's preferred to use pointers (perhaps with a `non_null` annotation). That makes it easy to write `{&source, &destination, &filter, &temp}` if needed, therefore making this better. (Alternatively, you can add a ctor.) – lorro Feb 20 '23 at 14:27
  • 1
    `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. – KamilCuk Feb 20 '23 at 14:28
  • @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. – lorro Feb 20 '23 at 14:31
  • 1+ for the nice fresh and juicy profile orange. It looks so delicious and matches the Stack Overflow colors so well. – Thomas Weller Feb 20 '23 at 14:37
  • Note: I will not downvote this as it has its uses. However, I will also not accept this as an answer as I deem it way too easy to misuse. – Martin Ba Feb 20 '23 at 15:00
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.

Dima
  • 38,860
  • 14
  • 75
  • 115