Cppcoreguidelines: Please don't "leave it to the reader" (P.9)

[IanEmmons]

In P.9, "Don't waste time or space", you show an example involving the tolower() function, and then you say, "We leave it to the reader to figure out what's wasted." Please don't do this. As stated in the abstract, "one purpose of a guideline is to help someone who is less experienced or coming from a different background or language to get up to speed." When a reader doesn't see the reason for this example, you are implicitly telling them that they aren't good enough to use C++, in other words that beginners and learners are not welcome.

[jwakely]

It doesn't mean that, it means the same as "can you see why this is a bad example?"

Your reading of it suggests that every textbook with questions at the end of each chapter is telling the reader they're not welcome.

And this one is _really_ easy. Don't repeatedly call the same function with the same input if the result will be the same every time. That's a waste of time.

But people do keep reading this in a way to take offense (see #879 as well) so maybe it should be changed.

[galik]

I think saying that "you are implicitly telling them that they aren't good enough to use C++" is way too strong. But I do agree that the focus in guidelines should be serving information, not presenting puzzles to solve (however easy they might seem). Puzzles might be better in a tutorial.

[scraimer]

Oooh, can we have a discussion here about what it _should_ be?

For context, here are the paragraphs under discussion:

Example, bad

void lower(zstring s)
{
    for (int i = 0; i < strlen(s); ++i) s[i] = tolower(s[i]);
}

Yes, this is an example from production code.
We leave it to the reader to figure out what's wasted.

I'd like to:

1. Explain the problem of calling strlen each time.
2. Add an example ("Example,good") that fixes that.

Example, bad

void lower(zstring s)
{
    for (int i = 0; i < strlen(s); ++i) s[i] = tolower(s[i]);
}

Yes, this is an example from production code.
Can you see the problem?
There will be a call to strlen on every iteration of the loop. The loop doesn't modify the string, which means the length of the string won't change. So recalculating the length of the string on every iteration is a waste.
To avoid that waste of time, hoist such constants out of the loop.

Example, better

void lower(zstring s)
{
    const auto len = strlen(s);
    for (int i = 0; i < len; ++i) s[i] = tolower(s[i]);
}

That still leaves a comparison between signed and unsigned, which also comes with its own costs. But if we go there, that might be reaching toward the realm of optimization. In which case, we'd have to consider reversing the direction of the loop, to take advantage of the lower cost of comparing to zero. I think for this section, that's overkill.

[martinmoene]

Phrased idiomaticallier : i != len;.

[AndrewPardoe]

We might also question the return value of the function and/or the type of the parameter.

[scraimer]

@AndrewPardoe huh, that didn't even occur to me.
If zstring doesn't hold a pointer to a memory block that is shared, then this function won't have any side effects, and is a prime candidate of being optimized away.

That leads me to an absurd conclusion: The original bad example was showing that even the worst-written code can waste very little CPU, if it's optimized away. That seems backwards, though.

[scraimer]

@martinmoene I'm not familiar with i != len being more idiomatic that i < len.
Perhaps you meant it produces better assembly output? I checked with Godbolt, and it seems to be the same output for -O2.

The only reason I left it as < instead of != is _habit_. I'm more used to seeing the < comparison. So I'd be delighted to understand why one is better (more correct / more performant / more expressive / readable / etc) than the other.

[scraimer]

@AndrewPardoe Would you rather we also suggest re-writing the function to not work in place?

Original declaration: void lower(zstring s)

Option 1: Rename the function to be void lower_in_place(zstring s)

Option 2: Rewrite the function so it doesn't modify the input argument: string lower(czstring s)
I don't like this because now the function does something different than the original function. It doesn't feel like a fix, but a full refactor. Perhaps the intent really _was_ to modify the string in-place.

Option 3: Rewrite the function so it requires different arguments: string lower(string const s)
This doesn't match the original intent of the function, with regard to what arguments it expects, and possible overhead.
If this is the better way to write it, this function should be replaced with std::transform and std::tolower. Which doesn't seem to be a good drop-in replacement for the original function, while possibly improving readability. While it might cost in performance (since the optimizers still have a hard time with std::transform) that will probably be solved over the next few years.

[tlanc007]

@scraimer Yes, I am still not use to != over i < len. The first I started seeing the former usage was in A Tour of C++. My guess is that the former notation makes it more visually consistent as the notation used with testing against iterators. ie pos != std::end(container).

But for me personally, the i < lenexplains the logic more precisely. One is stating that the range is [0,len) and that the loop should terminate for any value equal to or greater than len.

For example, the terminate condition, != wouldn't be be met in cases where the looping variable was allowed to exceed len. But then I suppose, in such an example, while/do-while loops would be more appropriate than using for loop.

[jwakely]

< only works for loops over integer indices and RandomAccessIterators, != works for those types and all other iterator types.

[scraimer]

Ah - iterators! That is the idiom.
I am enlightened! Thank you both.

[gdr-at-ms]

Sometimes, it is good to let people think.