[libcxx-commits] [PATCH] D121216: [libc++] Enable modernize-loop-convert

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/include/locale:3224-3226
+    for (char __p : __pat.field)
     {
+        switch (__p)
----------------
IMO we don't want to do this.
- Subjectively, to me, the old code looks easier to read at a glance. Here I'd have to scroll up to figure out what `__pat.field` is.
- This is a C++03 codepath, but ranged for loops are C++11. I guess obviously Clang supports them as an extension, and we have a general practice of permitting Clang's extensions in our "C++03" codepaths, so this bullet point should not be a blocker IMO.
- I'm shocked that this is the //one// place in all of libc++ that the machine thinks would benefit from a ranged for loop. This leads me cynically to suspect that the machine has a lot of false negatives right now. And //that// leads me to worry that like six months from now, clang-tidy is going to get improvements and start flagging all over libc++ and we're going to have to turn off `modernize-loop-convert` because now it's too noisy.
- And //that// illustrates my repeated point about all this: **If you weren't making this diff just to appease the machine, would it be a good diff in its own right?** Six months from now, when we turn off clang-tidy, are we also going to wish we could revert this one diff? (Obviously //I// think so.) If so, then we shouldn't make this diff now, either. We should use the machine to support libc++ style; we shouldn't contort libc++ style to please the machine.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121216/new/

https://reviews.llvm.org/D121216