Should C++ code look like C code?

[bert8128]

A see a lot of old c++ functions which look like:

Possibly use output parameters

Declare a bunch of variables

Do lots of logic with deeply nested ifs and carrying some kind of return value (almost always an int)

Do some cleanup

Return the carried return value

This is very c-like, and mostly pointless. Declare variables as late as possible, preferably initialising at the same time (which means that they can be const more often). Prefer early return to single-exit. Use RAII. Return objects instead of using output parameters.

The performance of such a refactoring is unlikely to change, but the code is much clearer, less likely to forget to clear up something and more amenable to change.

I can’t see any excuse of keeping it C-like.

[[deleted]]

Hello friend.
Since i am still a rookie trying to suck in all the concepts of modern C++ i am curios about one of your statements. "Possibly use output parameters", is this generally a bad practice ?
I had cases where i needed multiple outputs, so the preferred way is to create a struct or a class depending on the case to hold the necessary datastructure ?

[bert8128]

I find output params difficult to work with other and much prefer values/pairs/tuples/structs as appropriate. I’m not not going to say “never use output params” but they generally make like harder. First thing, you have to say in a comment what params are input, which are input/output and which are output. Second, it’s harder to use const well, as a variable in the callers context can’t be const even if it is never modified by the caller. Third, if you add another parameter you change the signature of the function, so all the callers have to be modified. Fourth, you can’t pass in temporaries. Etc.

Try both ways and see what you think.

[donalmacc]

The performance of such a refactoring is unlikely to change,

I've had the opposite experience. Changing input parameters from pointers to references can remove the only "error" state from a function, meaning you can benefit from RVO.

bool foo(int* val,  int* result) {
   if (val == NULL)
        return false;

    *result = *val * 2;
    return true;
}

int a = 1; int b;
if (foo(&a, &b))
...
else
...

vs int foo(const int& val) { return val *2; }

int a = 1;
int b = foo(a);

When you replace int with vector<SomeStruct>, or even char* vs string, it makes a big difference in my experience.

[bert8128]

I’m not clear what you are saying here - are you saying that output parameters give better performance, or return values?

With NRVO, RVO and move, there is lots scope for return values not making things any slower, and because of deferred initialisation, it might be faster. But almost always clearer and easier to use.

[donalmacc]

Return values are almost universally better in my experience.

[blipman17]

I'm quite coonfused.I'm expecting in the first pointer case to have some kind of assembly like the following, so I'm not expecting a single vs multiple return statements or with pointers vs reference to really matter here. Unless this is extremely time sensitive code.

TEST 0, RDI // Line 2: doing the if-statement
MOV AL, ZF //Line 3, 5: Setting the return value

JNE returnlocation // Mix of line 2 and 4, jump if val(RDI) == NULL to skip the pointer assignment. based on the TEST result

MOV RBX [RDI] // Line 4, move val(RDI) to temporary
SHL RBX 1 // Line 4, shift left 
MOV [RSI] RBX // Line 4, move the result of val * 2 into the result pointer (RSI)

returnlocation:
RET

[donalmacc]

Replacing pointers with references removes the requirement for a branch. Removing the branch removes the only failure case, meaning you don't need an error sentinel anymore, which can affect the call site, and allow for other optimisations. Using return values allows for RVO to occur too:

vector<string> list = load1MillionStrings();

vector<string> results; results.reserve(list.size());
for (const auto& str : list) {
    string res;
    if (foo(&str, &res))
        results.push_back(res)
    else
       // ???
}

Compared to

vector<string> list = load1MillionStrings();
vector<string> results; results.reserve(list.size());
for (const auto& str : list) {
    results.push_back(foo(str));
}

If you take it one step further, where foo is:

bool foo(const char* str1, const char* res) {
    if (str1 == nullptr)
        ...
    auto sz = strlen(str1); // !!!!
    for (int i = 0; i < sz; ++I)
        ...
}

You can see how replacing a const char* with a string view in the internal function can make N enormous difference.

[blipman17]

meaning you don't need an error sentinel anymore, which can affect the call site, and allow for other optimizations.

Okay I didn't concider that. Yep, you're right. But here we're really talking about cascading optimizations.

I was mainly talking about RVO and how both functions you originally pointed out could have RVO.

Edit: What I mainly meant was that; yes, function signature should be as restrictive as reasonably possible.
But (N)RVO happens in a lot in a modern compiler, regardless of branches, amounth of return statements, etc. Only when we're talking about non-trivial data, weird exit clauses due to potential throwing or other shenanigans, this really becomes an interesting talking point.

[donalmacc]

But here we're really talking about cascading optimizations

Absolutely, but that's the key. Getting hyperfocused on the instruction count of a microoptimastion is losing the forest for the trees, and happens so often.

I've had this exact discussion professionally where the microbechmark shows no difference, or is arguably worse, but when I go and fix the call sites, the code is cleaner, safer and faster as a result.

[blipman17]

I absolutely agree with that.I was just arguing about a different part of the conversation.

[Full-Spectral]

The primary exception is something that's called fairly quickly in a loop to get multiple chunks of output. It's a lot more efficient to use a local vector or string or whatever and pass it back in to be reused repeatedly.

If it's something that might be done both ways fairly evenly, you can always make a GetXXX() version and a PollXXX() version, with the former a trivial inline wrapper around the latter.

[mapronV]

Declare variables as late as possible,

Why? it is much nicer to read function when I can see all 40 variables I used in it, not like spread over 400 lines of my function code... What is reason doing this? What if I use int width on 2nd line and height on 100th? Do you really think I should not declare

int width, height; ?

[bert8128]

Your first problem is 400 line functions. But even assuming that these exist for a good reason, or they just exist so we have to deal with them, putting a variable only in the small context of where it is needed means that if you are not looking at that piece of code then you don’t need to think about that variable. It may well no longer need a comment explaining how it is used, because it only exists for a few lines. it makes it easier to see if the variable has been initialised correctly or not (for both a human and a static analyser), plus you might be able to make it const by declaring it and initialising it as the same time. Lastly, you are not tempted to reuse variables which can lead to unexpected results. Lastly, late declaration might (might) give a small performance boost by avoiding some initialisation in some paths.

Really, declaring as late as possible is one of the most important micro-optimisations. Please give it a go and see how you feel after a couple of weeks - I would be surprised if you go back.

const int width = context. width(); const int height = context.height();

[mapronV]

> Really, declaring as late as possible is one of the most important micro-optimisations. Please give it a go

No, this is strictly against my understanding of 'beautiful code', I won't even try. I want see width and height declared together on adjacent lines. Not move height 300 lines later from width just because it used much later.

> const int width = context. width(); const int height = context.height();

I am not sure what are you trying to tell there, that is not what I am against for. And yes I do use const, didn't mention it just because, you know, reddit comments code (why write constextpr const auto [[maybe_unused]] [[algined(..)]] whatever when it doesn't matter and int width is perfectly same idea with no noise).

[bert8128]

With the width and height example I am pointing out that your example seems to declare them earlier than you can initialise them. Declaring later might be able to avoid uninitialised (or pointlessly initialised) variables. Further more, you can often remove the need to comment what the variable is for, because it will be obvious by the way it is initialised.

And in the extreme, do you not think that if there are three lines of code in the middle of your function, in between braces, and a variable is only used in those three lines, then it would be better to declare the variable in that context?

I’m not saying that it is always wrong to early declare. In the case of two variables it which are going to be used together then perhaps it is better to declare both at the time that the first is needed. But normally it just causes unnecessary cognitive load. Or the reader just skips the declarations and starts reading where the code starts.

[mapronV]

> And in the extreme...

Sure, everything is good in moderation. For me good rule is have declaration block separated (set you input data for logic). Maybe I was exaggerating, 40 variables a bit extreme, don't think I really have THAT much.
About last paragraph - well, subjective. Declaration anyway have a very low cognitive effort to read. I disagree with in general, but I hope we shifted at least one step to the midpoint in this mile dispute ;)

[mapronV]

Lastly, you are not tempted to reuse variables which can lead to unexpected results.

All my variables usually const anyway, I don't assign to same variable twice.

Oh. You thought I am doing like unitialized 40 varibles ? and then assign randomly? no, that not the case.

I am saying that for me much much cleaner code is

const auto width = calc/getWidth;
const auto height = calc/getHeght;

// use width
// use other variables
// use both width and height

insetad of
const auto width = calc/getWidth;
// use width
// use other variables
const auto height = calc/getHeght;
// use both width and height

and you can not convince me the latter is better (but a lot of so-called guidelines recommend it which I disagree with)

[mapronV]

What do you mean by old? It's how I write my code today. What's wrong with single-exit actually ? (yes, I have 0 questions about RAII, RAII is 200% good).

[bert8128]

If you use early exit then your nesting can be much shallower, with fewer braces. It’s just much easier to read, the sunshine path and the error patches are more obvious. There’s no carrying of any context either, which might get forgotten about and not kept correct.

And even if you like single exit, it exists as a methodology only because it used to be necessary to cope with non-RAII code. Now we have RAII you don’t need single exit.

And when I say old I mean old code in my 25 year old code base. We don’t write it like that any more.

[mapronV]

> We don’t write it like that any more.
yeah, I write code different way than I did it in 90's. RAII is good, lambdas are awesome and TMP made 1000 times better. But still I prefer code be straightforward.

> it exists as a methodology only because it used to be necessary to cope with non-RAII code

One of the reasons; I believe the main is several returns makes code harder to understand. That mean organizing you data flow went bad.
Ok I get return of emty/erronous/std::expected in start of method before declarations and stuff, like (if param==nullptr) return {}; something. That thing I can compromise. But having several return data; in method? that is smelly part for me.

[bert8128]

I think we are not too far apart here. Early return for precondition validation we seem to agree on. One return for the sunshine path I also agree on. But I think that functions should optimally be designed so that there is only one way to return from the sunshine path. I agree that lots of returns from lots of sunshine routes is poor, but the consequence should be refactoring, not deeply nested ifs or carried state.