[webkit-dev] [webkit-reviewers] usage of auto

[Darin Adler]

> On Jan 10, 2017, at 10:17 PM, Filip Pizlo <fpizlo at apple.com> wrote:
> 
>             while (Arg src = worklist.pop()) {
>                 HashMap<Arg, Vector<ShufflePair>>::iterator iter = mapping.find(src);
>                 if (iter == mapping.end()) {
>                     // With a shift it's possible that we previously built the tail of this shift.
>                     // See if that's the case now.
>                     if (verbose)
>                         dataLog("Trying to append shift at ", src, "\n");
>                     currentPairs.appendVector(shifts.take(src));
>                     continue;
>                 }
>                 Vector<ShufflePair> pairs = WTFMove(iter->value);
>                 mapping.remove(iter);
> 
>                 for (const ShufflePair& pair : pairs) {
>                     currentPairs.append(pair);
>                     ASSERT(pair.src() == src);
>                     worklist.push(pair.dst());
>                 }
>             }

Here is the version I would write in my favored coding style:

    while (auto source = workList.pop()) {
        auto foundSource = mapping.find(source);
        if (foundSource == mapping.end()) {
            // With a shift it's possible that we previously built the tail of this shift.
            // See if that's the case now.
            if (verbose)
                dataLog("Trying to append shift at ", source, "\n");
            currentPairs.appendVector(shifts.take(source));
            continue;
        }
        auto pairs = WTFMove(foundSource->value);
        mapping.remove(foundSource);
        for (auto& pair : pairs) {
            currentPairs.append(pair);
            ASSERT(pair.source() == source);
            workList.push(pair.destination());
        }
    }

You argued that specifying the type for both source and for the iterator helps reassure you that the types match. I am not convinced that is an important interesting property unless the code has types that can be converted, but with conversions that we must be careful not to do. If there was some type that could be converted to Arg or that Arg could be converted to that was a dangerous possibility, then I grant you that, although I still prefer my version despite that.

You also said that it’s important to know that the type of foundSource->value matches the type of pairs. I would argue that’s even clearer in my favored style, because we know that auto guarantees that are using the same type for both, although we don’t state explicitly what that type is.

Here’s one thing to consider: Why is it important to see the type of foundSource->value, but not important to see the type of shifts.take(source)? In both cases they need to be Vector<ShufflePair>.

— Darin