jrtc27 added inline comments.
================ Comment at: llvm/test/CodeGen/RISCV/loop-alignment.ll:3-4 +; RUN: llc < %s -mtriple=riscv64 | FileCheck %s +; RUN: llc < %s -mtriple=riscv64 -align-loops=16 | FileCheck %s -check-prefix=ALIGN_16 +; RUN: llc < %s -mtriple=riscv64 -align-loops=32 | FileCheck %s -check-prefix=ALIGN_32 + ---------------- asb wrote: > MaskRay wrote: > > luismarques wrote: > > > MaskRay wrote: > > > > jrtc27 wrote: > > > > > MaskRay wrote: > > > > > > jrtc27 wrote: > > > > > > > MaskRay wrote: > > > > > > > > luismarques wrote: > > > > > > > > > Nit: it's a convention of the RISC-V backend codegen tests to > > > > > > > > > wrap the RUN lines. > > > > > > > > only 86 columns. compiler-rt is even transiting to 100 column. > > > > > > > compiler-rt is not the RISC-V backend :) > > > > > > Wrapping lines here just makes the code less readable. > > > > > That's your personal opinion, which I disagree with, and it's not > > > > > true if your terminal isn't wide enough. Going against existing > > > > > convention in the backend tests should only be done with very good > > > > > reason, and personal opinion is not that. > > > > Lines longer than 80-column (in this case just 86) are pretty common > > > > among tests. I really hope test/CodeGen/RISCV/ can be more tolerant on > > > > this matter. > > > > > > > > Even the Linux scripts/checkpatch.pl has increased the limit to 100 > > > > because in many cases wrapping lines for strict 80-conformance just > > > > harms readability. > > > > > > > > Of course I don't want to waste time arguing on this matter. So if this > > > > turns out to be an issue for RISC-V folks, I'll update it to save my > > > > time. > > > > Of course I don't want to waste time arguing on this matter. So if this > > > > turns out to be an issue for RISC-V folks, I'll update it to save my > > > > time. > > > > > > Personally, I don't particularly care. I don't know if @asb has strong > > > feelings about this. If you think it would be beneficial to relax this > > > convention please raise the issue on llvm-dev. Let's not keep discussing > > > this in every patch touching RISC-V :-) > > Personally I don't even think the generic case needs to be raised on > > llvm-dev:) There are just so many column>80 cases in llvm/test and > > clang/test. Actually, If someone wants to enforce the 80-column rule more > > rigidly, that probably needs a discussion. > > > > That said, the argument here is about a subdirectory: > > llvm/test/CodeGen/RISCV/ ... > > > > > I don't have a strong view on this one to be honest. I think I've typically > wrapped at 80 columns for these RUN lines after being asked to, but > ultimately I think choosing a logical point to split has a greater impact on > readability than keeping it strictly to 80 columns. FWIW I care less about argument lists extending beyond 80 columns, but I do think the | is a logical point at which to wrap it if you have a long line and keeps things more readable. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D106701/new/ https://reviews.llvm.org/D106701 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits