On Jan 10, 2017, at 23:49, Darin Adler <[email protected]> wrote:
>> On Jan 10, 2017, at 10:17 PM, Filip Pizlo <[email protected]> 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. It would take me longer to understand your version. The fact that the algorithm is working on Args is the first thing I'd want to know when reading this code, and with your version I'd have to spend some time to figure that out. It actually quite tricky to infer that it's working with Args, so this would be a time-consuming puzzle. So, if the code was changed in this manner then I'd want it changed back because this version would make my job harder. I get that you don't need or want the type to understand this code. But I want the type to understand this code because knowing that it works with Args makes all the difference for me. I also get that conversions are possible and so the static assertion provided by a type annotation is not as smart as it would be in some other languages. But this is irrelevant to me. I would want to know that source is an Arg and pair is a ShufflePair regardless of whether this information was checked by the compiler. In this case, putting the information in a type means that it is partly checked. You could get it wrong and still have compiling code but that's unlikely. And for what it's worth, my style is to prefer explicit constructors unless I have a really good reason for implicit precisely because I like to limit the amount of implicit conversions that are possible. I don't think you can implicitly convert Arg to anything. > > 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. I agree with you that anytime you use auto, it's possible to unambiguously infer what the type would have been if you think about it. I want to reduce the amount of time I spend reading code because I read a lot of it. It would take me longer to read the version with auto because knowing the types is a huge hint about the code's intent and in your version I would have to pause and think about it to figure out the types. > > 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>. Your version does not annotate the type at all. I would have to guess that the pair is a ShufflePair and that pairs is a Vector of ShufflePairs. I agree that probably anyone working on WebKit would be smart enough to solve this puzzle. I don't want my job to involve solving more puzzles than it already does. The point of saying the type explicitly is that instead of spending time to solve this puzzle, you can instead see the type at a glance and move on to understanding the algorithm. -Filip _______________________________________________ webkit-dev mailing list [email protected] https://lists.webkit.org/mailman/listinfo/webkit-dev
