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

[Adam Majer]

On 03/15/2017 11:43 AM, khaled belhout wrote:
> hello Alex
> I fixed the naming of the current iteration object, limited the
> changes to "src/adaptation" sub tree and replaced NULL macro by
> nullptr keyword.

I don't want to be too critical about this, but these types of changes
do not add anything. They actually make the code more difficult to maintain.

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?


-Adaptation::Message::Message(): header(NULL)
+Adaptation::Message::Message(): header(nullptr)

NULL and nullptr are the same thing for these purposes. Actually, the
most likely implementation of NULL macro in C++11 is just

#define NULL nullptr

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

Another example,

     typedef Master::Extensions::const_iterator MECI;
-    for (MECI i = master.extensions.begin(); i !=
master.extensions.end(); ++i) {
-        if (name == i->first)
-            return Area(i->second.data(), i->second.size());
+    for (const auto & extension : master.extensions) {
+        if (name == extension.first)
+            return Area(extension.second.data(), extension.second.size());
     }

In this example, you've not even removed MECI typedef. But basically the
point here is 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.

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.

This is where you *may* want to use auto, like

std::vector<std::pair<int, std::unique<Something>>> my_awesome_vector;
for (const auto &p : my_awesome_vector) {
    ....
}

But if you have,

std::vector<Something> some_vector;
for (const Something &thing : some_vector) {
   ...
}

because using

for (const auto &thing : some_vector) {
   ...
}

doesn't really save you anything, it just makes it worse.

These are just my personal opinions. Others may disagree.

- Adam