<div dir="ltr"><div dir="ltr"><div><div><br></div></div><div>For the vote count, I'm also in the group that we should format everything. Even if you aren't making modifications to the code, it's difficult to read code you're trying to debug if it's in a different style. Of course, these formatting changes should go through human review, and we should hold off on problematic places until we can fix clang-format to deal with it better (or use tricks like comments for alignment).</div><div><br></div><div>For the argument about downstream forks experiencing churn -- is it enough to say that if your downstream fork hits a merge conflict, just back out of the merge and clang-format those files in your downstream fork, and your next attempt to rebase should usually work?</div><div><br></div><div>I think posting some clang-formatting patches, and looking at more "bad" examples might help. There are usually workarounds:</div><div><br></div></div><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Jun 30, 2020 at 1:56 PM Nicolai Hähnle via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org">llvm-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">... </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Since you're interested in thoughts about clang-format's capabilities,<br>
I agree with Matt that the strictness of the approach is a weakness<br>
that can frequently make code *less* readable. In addition to what he<br>
mentions, here's an example of a bad change that clang-format wants to<br>
make that I found after half a minute of scanning our AMDGPU backend<br>
code:<br>
<br>
// We only support LOAD/STORE and vector manipulation ops for vectors<br>
// with > 4 elements.<br>
- for (MVT VT : { MVT::v8i32, MVT::v8f32, MVT::v16i32, MVT::v16f32,<br>
- MVT::v2i64, MVT::v2f64, MVT::v4i16, MVT::v4f16,<br>
- MVT::v4i64, MVT::v4f64, MVT::v8i64, MVT::v8f64,<br>
- MVT::v16i64, MVT::v16f64, MVT::v32i32, MVT::v32f32 }) {<br>
+ for (MVT VT :<br>
+ {MVT::v8i32, MVT::v8f32, MVT::v16i32, MVT::v16f32, MVT::v2i64,<br>
+ MVT::v2f64, MVT::v4i16, MVT::v4f16, MVT::v4i64, MVT::v4f64, MVT::v8i64,<br>
+ MVT::v8f64, MVT::v16i64, MVT::v16f64, MVT::v32i32, MVT::v32f32}) {<br>
for (unsigned Op = 0; Op < ISD::BUILTIN_OP_END; ++Op) {<br>
switch (Op) {<br>
<br>
I don't particularly mind that clang-format puts that initializer list<br>
onto a new line, or that it changes the whitespace around braces. What<br>
I do mind: the original code lays the initializer list out carefully<br>
such that integer and floating point types always come in pairs on the<br>
same line: v8[if]32 and v16[if]32 on the first line, v2[if]64 and<br>
v4[if]64 on the second line, and so on. clang-format mindlessly mushes<br>
the initializer list together in a way that makes it harder to see at<br>
a glance what is going on.<br></blockquote><div><br></div>A common strategy for this type of pattern is to use trailing comments for alignment:<div><br><font face="monospace"> for (MVT VT : {<br> MVT::v8i32, MVT::v8f32, MVT::v16i32, MVT::v16f32, //<br> MVT::v2i64, MVT::v2f64, MVT::v4i16, MVT::v4f16, //<br> MVT::v4i64, MVT::v4f64, MVT::v8i64, MVT::v8f64, //<br> MVT::v16i64, MVT::v16f64, MVT::v32i32, MVT::v32f32 //<br> }) {<br></font><div><div><br></div><div>Live example: <a href="https://godbolt.org/z/PehFcC">https://godbolt.org/z/PehFcC</a></div></div><div><br></div><div>(( Of course, even better, you can add text to those comments, like "// v8, v16". But it's a common enough pattern to use empty alignment comments. ))</div><div></div></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
(Note that this isn't code that I wrote, so I'm not emotionally<br>
attached to it or anything. But I've run into this kind of problem<br>
many times during development.)<br>
<br>
I believe the common theme here is that clang-format ought to have a<br>
mode in which it is more accepting of different layouts of lists<br>
co-existing within the same code base. If that feature was available,<br>
I would be a strong proponent for adopting it in LLVM itself.<br></blockquote><div>Does "// clang-format [ on | off]" suffice as an escape hatch?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Cheers,<br>
Nicolai<br>
<br>
<br>
<br>
<br>
<br>
<br>
> I realize it feels like unnecessary churn which is why I suggested this be in code which hasn't been touched in some time (I'd be fine if that time is 1+ years), but more often than not this is quite small basic style issues, similar to the ones below.<br>
><br>
> MyDeveloperDay<br>
><br>
> - void HandleTranslationUnit(ASTContext& context) override {<br>
> + void HandleTranslationUnit(ASTContext &context) override {<br>
> if (!Instance.getLangOpts().DelayedTemplateParsing)<br>
> return;<br>
><br>
> - std::set<FunctionDecl*> LateParsedDecls;<br>
> + std::set<FunctionDecl *> LateParsedDecls;<br>
><br>
> On Tue, Jun 30, 2020 at 6:55 PM Matt Arsenault <<a href="mailto:arsenm2@gmail.com" target="_blank" class="cremed">arsenm2@gmail.com</a>> wrote:<br>
>><br>
>><br>
>><br>
>> On Jun 28, 2020, at 11:30, MyDeveloper Day via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank" class="cremed">llvm-dev@lists.llvm.org</a>> wrote:<br>
>><br>
>> I’m a contributor to clang-format and would like to see LLVM 100% clang formatted so we can use LLVM as a massive test-suite for clang-format when we make changes.<br>
>><br>
>><br>
>><br>
>> My main issue with this would be that clang-format does things that I don’t believe are stated in the LLVM style guide and I also disagree with. There’s a whole set of cases where it makes unwelcome changes to put things that were on separate lines on a single line. Whenever I run clang-format, I typically git checkout -p to discard all of these.<br>
>><br>
>> For example, it likes to take member initializer lists and pack them into as few lines as possible. This is just asking for merge conflicts (e.g. AMDGPUSubtarget has a ton of fields in it, and out of tree code is constantly adding new fields for unreleased subtargets). It also mangles BuildMI calls, where I believe every .add() should be on its own line, and stringing this into BuildMI().addFoo().addBar() is way less readable.<br>
>><br>
>> I also believe it’s 4 space indent on line wraps differs from the stated 2 space indent level (and this also disagrees with emacs llvm-style)<br>
>><br>
>> -Matt<br>
><br>
> _______________________________________________<br>
> LLVM Developers mailing list<br>
> <a href="mailto:llvm-dev@lists.llvm.org" target="_blank" class="cremed">llvm-dev@lists.llvm.org</a><br>
> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank" class="cremed">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
<br>
<br>
<br>
-- <br>
Lerne, wie die Welt wirklich ist,<br>
aber vergiss niemals, wie sie sein sollte.<br>
_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:llvm-dev@lists.llvm.org" target="_blank" class="cremed">llvm-dev@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank" class="cremed">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
</blockquote></div></div>