[PATCH] D68554: [clang-format] Proposal for clang-format to give compiler style warnings

Thu Oct 10 00:20:41 PDT 2019

klimek added a comment.

In D68554#1702090 <https://reviews.llvm.org/D68554#1702090>, @MyDeveloperDay wrote:

> As this behaviour is not something clang-format should do because there are external tool that can do this for us, I thought I'd set myself a challenge of trying to write this with external tools, to prove to myself that it was indeed easy, what I want to achieve is being able to go to a directory and determine if there are any clang-format changes needed in the source files of the levels below with a single command.
>
> The idea would be that this could be something that could fit on the command line, or made into a bash alias/function (more likely), I really don't want to turn to python as if I'm coding this in another language I might as well be coding it in C++ inside clang-format and it opens me up to the whole can't find the correct python headache. (but I also think the clang diagnostic classes give doing it inside C++ the edge)
>
> I want to take into account at least the concepts from D29039: [clang-format] Proposal for clang-format -r option <https://reviews.llvm.org/D29039> about being able to look deeply into a directory.
>
> There are a couple of requirements: (some nice to have's)
>
> 1. look deeply into a directory for all files supported by clang-format (C++,C# etc)
> 2. file extensions should be case insensitive i.e. .cpp or .CPP
> 3. be able to clearly see the name of  the file that need clang-formatting
> 4. don't actually do the formatting (tell me what needs doing but don't necessarily make the change)
> 5. ideally show a snippet of the file where the change is needed (I guess this isn't essential)
> 6. This should be cross platform windows, linux, (I guess other *nix's) it should work in at least the common shells bash,cygwin,powershell
>
>   The cross platform is the real killer, this forces you down the python road, (or the C++) if you want a common solution otherwise you are implementing different solutions on different plaforms.
>
>   But lets ignore that and lets start with a Linux or Cygwin bash shell. Surely the best way to find all source files that can be converted is via a find. In the past I've only ever used find like this to find multiple extensions
>
>   ```find .\( -name "*.h" -o name "*.cpp" \) -print ```
>
>   But this is really unworkable for so many output formats that clang-format supports, and even if you could get that to work it would be case sensitive unless you uses -ilname which would make the command line even longer
>
>   So lets try using -iregex, this is a much shorter case insensitive command but still, it needs a certain amount of escaping in the regular expression, (lets hope we never do "clang-format all the things") as this could get longer.
>
>   ```find . -iregex '.*\.\(cpp\|cc\|c\+\+\|cxx\|c\|cl\|h\|hh\|hpp\|m\|mm\|inc\|js\|ts\|protor\|protodevel\|java\|cs\)' -print```
>
>   if I want to put that into an alias then I need switch the ' and " around
>
>   ```alias find_all_code='find . -iregex ".*\.\(cpp\|cc\|c\+\+\|cxx\|c\|cl\|h\|hh\|hpp\|m\|mm\|inc\|js\|ts\|protor\|protodevel\|java\|cs\)" -print'```
>
>   Is the same thing as easy in powershell? Ok not really .. this is as far as I got, and I couldn't work out how to handle multiple extensions let alone case sensitivity.. (it feels very slow too!)
>
>   ```PowerShell.exe -Command "dir -Recurse -ea 0 | ? FullName -like '*.cpp'```
>
>   So at least my brief initial conclusion is... if we are not using C++, its Python or nothing if you want cross-platform or maybe find if you only care about *nix
>
>   But even the find... that only covers 1) and 2) of my requirements...how do I determine even with find which files need formatting..
>
>   This command would show me the contents of everything that needs changing.
>
>   ```find . -iregex '.*\.\(cpp\|cc\|c\+\+\|cxx\|c\|cl\|h\|hh\|hpp\|m\|mm\|inc\|js\|ts\|protor\|protodevel\|java\|cs\)' -exec clang-format {} \;```
>
>   This would give me the list of changes, but then I lose the name of the file.
>
>   ```find . -iregex '.*\.\(cpp\|cc\|c\+\+\|cxx\|c\|cl\|h\|hh\|hpp\|m\|mm\|inc\|js\|ts\|protor\|protodevel\|java\|cs\)' -exec clang-format --output-replacements-xml {} \;```
>
>   So using external tools is easy how? the dumping of the output to stdout or the replacement xml to stdout makes this much harder as I lose the filename that was passed to clang-format, (there is probably an xargs trick here I'm missing)
>
>   This proposed solution would make all this possible just with:
>
>   ```find . -iregex '.*\.\(cpp\|cc\|c\+\+\|cxx\|c\|cl\|h\|hh\|hpp\|m\|mm\|inc\|js\|ts\|protor\|protodevel\|java\|cs\)' -exec clang-format -i -n {} \;```
>
>   and more to the point the list of file types supported by clang-format is something clang-format knows... why not combine this with D29039: [clang-format] Proposal for clang-format -r option <https://reviews.llvm.org/D29039> and just do
>
>   clang-format -r -i -n .
>
>   This provides in my view the only cross platform, shell independent way of doing this in what is a relatively minimal code change, and I'm struggling to see this as easy to do with an external tools? but if someone wants to show me how, I'd be interested to learn.

I think it's easy enough to do as a kind of driver, either in python or C++, but then again, at that point putting it into ClangFormat seems fine. One thing that I find on the one side kinda nice, but on the other side also weird is the names of the flags. Making them more similar to clang flags is nice from the point of people used to clang being able to quickly identify them, but the structure also makes it look like clang-format would support a wider range of flags, which it doesn't. Not sure which side I'm on, and I'm fine with the patch as is (minus pulling out a function), but wanted to bring it up if folks have ideas.

================
Comment at: clang/tools/clang-format/ClangFormat.cpp:371
+      if (!Replaces.empty()) {
+        IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts =
+            new DiagnosticOptions();
----------------
I'd pull this block out into a function.

Repository:
  rC Clang

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

https://reviews.llvm.org/D68554