MLIR has a custom clang-format config with a minor divergence from LLVM:
AlwaysBreakTemplateDeclarations: Yes
It ensure that we always have this:
template<>
void good();
Even when the declaration can all fit on one line:
template<> void bad();
I don’t see a strong reason to diverge from LLVM here, so I’d like to remove this.
If I remember correctly, last time I looked at this BTDS_MultiLine had a bug and this was to work around that.
So we should instead fix the bug in clang-format and update add a file .clang-format file to LLVM to support older clang-format version?
+1. I had tried ~year ago to look at it one Friday, but didn’t make any progress (beyond noticing this option didn’t not get checked in one internal part of clang-format where I would have expected it to change behavior).
[Sorry had replied via email to the thread, but seems I didn’t do it correctly.]
I just ran into a pre-merge check for (https://reviews.llvm.org/D76329) that failed I think because of the difference in these configs (https://reviews.llvm.org/harbormaster/build/54238/). It wants me to break the template declaration across lines. It would be great to unify these configs.
I think it fails because you didn’t clang-formatted using git clang-format but did it using some Google internal tool which is ignoring the configuration.
Yeah it’s just using the default LLVM configuration.
I’d like to see this unified as well since my default configuration is to have my editor automatically invoke clang-format whenever a file is saved. I have to do something ‘special’ when I work in the MLIR repository today. I feel like running this should yield no new differences compared to HEAD at any given time:
find mlir/ -name *.cpp | xargs clang-format-9 -i
If someone want to tackle this 
Isn’t the second format (breaking the template declaration) much more readable? It is to me. Irrespective of templatizing, you have a uniform/consistent way to start class/struct/func declarations. Since we diverge from LLVM coding style on certain aspects, could we retain this if it’s good? Although this is the only divergence as far as formatting / clang-format goes, the custom .clang-format takes care of it and tools find it.
Doesn’t your editor look for a .clang-format in the tree? So, for everything under mlir/, it should do the right thing.
Yes, the mlir/.clang-format it finds is the one that specifies that templates should have a newline. So whenever I hit save, it conforms to the checked in .clang-format. However, this format does not conform to what’s currently checked in, hence we see diffs in CLs.
Is your editor changing the whole files instead of only the diff?
Yes, my point of contention is that the .clang-format that is checked in should match the format of the rest of the code checked in.
Thus, I think we have two options:
.clang-format file..clang-format file so that it actually matches the format that is currently in use.I prefer option 1. Once you have a file open, it’s far more readable/easier to find definitions when template declarations are broken - because you don’t have to keep moving your eyes horizontally by variable amounts to find the " func/class name". IIRC, we went with an always BTD style because it was more readable - you just have to scan vertically.
This would be ideal, but this is hard to enforce: not everyone is using clang-format consistently and so there will always be some divergence. On this aspect, MLIR is in much better place than the rest of LLVM I believe.
Your proposal also assumes that there is a style in use: the current presubmit check is flagging any new code that wouldn’t follow the existing .clang-format and I expect most code checked-in the last 2 months to respect it.
While I’ve always preferred this solution for this kind of changes and I like uniformity in the code base, I have to point out that this is not the current guideline: https://llvm.org/docs/CodingStandards.html#introduction
we explicitly do not want patches that do large-scale reformatting of existing code. On the other hand, it is reasonable to rename the methods of a class if you’re about to change it in some other way.
So at the moment, the “blessed” way to format code is git clang-format HEAD~ and not formatting entire files at once.
That said, I also believe it is always better to consider such things individually instead of in the abstract, so here is what such a change would look like: https://reviews.llvm.org/D76821
I’m very +1 on aligning with LLVM here.
a. It removes the need for a custom clang-format
b. I personally find the second form much more readable.
The first one has objective benefit, the second one is subjective in either direction.
While I’ve always preferred this solution for this kind of changes and I like uniformity in the code base, I have to point out that this is not the current guideline: https://llvm.org/docs/CodingStandards.html#introduction
I personally like the LLVM approach here. I think it’s ok to allow discrepancies in the format in favor of making future format changes, such as this one, easy and less disruptive (i.e., without having to introduce massive changes all over the project). I think the latter point is really important to avoid large-scale conflicts with WIP patches and users that may have private customizations of the MLIR code.
So at the moment, the “blessed” way to format code is git clang-format HEAD~ and not formatting entire files at once.
+1. Applying formatting to the whole file could be a bit risky since even sometimes format changes along with the own evolution of the clang-format tool. That would lead to having commits with arbitrary unrelated changes often.