[PATCH] D29039: [clang-format] Proposal for clang-format -r option

Thu Oct 10 14:24:35 PDT 2019

MyDeveloperDay added a comment.

In D29039#1704166 <https://reviews.llvm.org/D29039#1704166>, @zturner wrote:

> In Windows you just write a Python script to do this, and this works everywhere, not just on one platform or the other, so bash isn't even necessary and Python is easy to write so I wouldn't really say it's "even harder".  If you google for `run-clang-format.py` you'll find a script that actually branches out and does this in parallel.  There's a lot of logic and smarts you could bake into an external tool which can then be used for many different programs, not just clang-format.

@zturner thanks for the feedback, I took a look and run-clang-format.py at (https://github.com/Sarcasm/run-clang-format), and whilst I have a reasonable knowledge of python, reading the python code just makes me ask why isn't all of this just in clang-format in the first place?

To me I think these are unaddressed clang-format requirements that other third-party developers are providing, without having to conform to some agreed upstream set of rules or gaining consensus. The author has coded it the way they want, how they want and can add to it to do what they want (good on them). But that doesn't mean that's the only approach or best choice of technology, or that someone should follow the same model or develop something similar. Its one option, a good option, but not the only option.

I've personally been burnt on Windows using a combination of python in both the python windows distribution and cygwin together, the windows version doesn't understand Cygwin paths (as expected). I know I do my own development in clang with 1 Visual Studio Command Prompt setup to pick up VS2019 and one bash prompt to ensure it picks up python correctly,  (I don't use CMake to make visual studio projects but use the Code Blocks CMake options as that works nicely with cygwin, allowing me to do a make -jN which is essential, the visual studio projects are pretty unworkable)

I have to build in the windows shell so it finds the correct compiler cl.exe and not the cygwin's gcc, but I can't run `git clang-format` or `git llvm push` in those windows shells as the python breaks. and that all comes down to the interaction of different pythons. (and I just cannot justify to myself proliferating that pain to others by using python as a solution when we have a perfectly good solution in C++) . Ask yourself how many people have trouble getting the lit tests to work in LLVM on windows I know I do. Which I'm sure is resolved by some configuration magic but I haven't found it yet.

But I am grateful for the evidence that needing this capability is obviously required (otherwise someone wouldn't have written this script)

if we take a look at the script what is it doing... the script is ~ 350 lines

1. A certain section is covering recursive running (about 20-40 lines)
2. A reasonable amount  is simply handling the command line arguments (30-50 lines)
3. About 40 lines is colourizing a diff
4. And about 70-100 lines is handling running in parallel

The command-line options it supports just:

--clang-format-executable (to tell it where to find the executable)
-extensions to tell it what to file types to handle (which actually their list is out of date as it only covers some C++ code and not js,C#,protobuf etc)
-r for recursive
-q for quiet 
-j for the number of parallel jobs
-color for showing a colored diff
--exclude for paths to exclude.

Of that which parts are useful? the colored diff? maybe.. the parallelizm is nice but that's not essential in my view on a fast machine and there are other ways of doing that especially in makefiles which are already running -j N maybe you could even use gnu parallel for that?

So basically we are down to --extensions, -r, --exclude ... as perhaps the real benefit, I think its this functionality that this revision is/should be adding. I think its relatively simple and is already partially covered in about 30+ lines above.

But the existence of this script just means I'm even more sold on the fact that we should do it..so I really appreciate you bringing that to my attention.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D29039