[squid-dev] Fwd: [PATCH] for loops modernization

[Alex Rousskov]

On 03/15/2017 06:57 AM, Adam Majer wrote:
> these types of changes
> do not add anything. They actually make the code more difficult to maintain.

I disagree.

> For example,
> 
> -    if (HttpRequest *r = dynamic_cast<HttpRequest*>(theMsg)) {
> +    if (auto *r = dynamic_cast<HttpRequest*>(theMsg)) {
> 
> Nothing is gained here. On the contrary, it is not immediately known to
> the reader what this `auto` type actually is until they look to the
> right. So what are we saving?

While we do have to "look to the right" and that is indeed a drawback,
auto advantages, collectively, outweigh that negative:

* Auto may reduce explicit code duplication. Saying HttpRequest twice is
code duplication. Code duplication is one of the greatest evils we have
to deal with on a daily basis. It causes functionality bugs and
significantly increases code maintenance overheads.

* Auto reduces implicit code duplication: Saying HttpRequest just
because the type returned by the expression on the right is currently
HttpRequest.

* Auto eliminates implicit type conversions. Some of those conversions
have very undesirable effects, including serious performance degradation.

* In most cases, you either do not need to look to the right (because
you do not care about the actual type specifics) or you have to look to
the right for other reasons (because you care about the code details
after "auto" so you have to keep reading anyway).

More on that below, after the nil pointer discussion (including a
reference to an "authoritative" analysis of your readability argument).


> -Adaptation::Message::Message(): header(NULL)
> +Adaptation::Message::Message(): header(nullptr)
> 
> NULL and nullptr are the same thing for these purposes.

They may or may not be (depending on the header declaration). Since NULL
and nullptr may have different types, they are not identical. More at
http://stackoverflow.com/questions/13816385/what-are-the-advantages-of-using-nullptr


> Actually, the
> most likely implementation of NULL macro in C++11 is just
> 
> #define NULL nullptr

According to the comments at the above URL, that would be illegal
because nullptr is not an integral type and NULL has to have an integral
type.


> I have no problem in using either, but changing this just for the same
> of changing it....

Yes, please do not misinterpret my comments as a disagreement on that
very important point: Nullptr is better than NULL, but the negatives in
just changing lots of old working (and needed to be supported across
various branches) code to nullptr outweigh the advantages.


> -    for (MECI i = master.extensions.begin(); i != master.extensions.end(); ++i) {
> +    for (const auto & extension : master.extensions) {

> [...] this makes is even more complicated to know the type you
> are referring to. The compiler knows, but the developer? I have no idea.
> master.extentions can be anything with iterator.

That is usually a good thing: You do _not_ want to know the exact type
in most cases.


> If you really want to fix this, avoid `auto` in code people actually
> want to maintain. The only place it is actually useful is in heavily
> templated code and that is the main reason it was introduced. It was not
> introduced so you can avoid typing the name of POS (plain old
> structures) or types or classes.

I disagree on all counts. Auto is much more nuanced. The web has a lot
more pro- and cons- arguments about auto, including advice from C++
gurus. For example:
https://herbsutter.com/2013/08/12/gotw-94-solution-aaa-style-almost-always-auto/

We should not repeat those debates here IMO. What would be the point? We
are unlikely to come up with better or new arguments... For example, the
above page already discusses your very own readability argument at length!

In my personal experience, going against Sutter and Mayers advice
usually ends up in long-term regrets. They are sometimes wrong, but it
is usually much easier to fix their wrongs than mine. (Just like with
Squid or any other development, it is often about making good mistakes
versus the bad ones.)


Cheers,

Alex.