This is an archive of the discontinued LLVM Phabricator instance.

Differential D106847

Revert "sanitizers: increase .clang-format columns to 100"
ClosedPublic

Authored by dvyukov on Jul 26 2021, 10:04 PM.

Details

Reviewers
melver
MaskRay
dblaikie
vitalybuka
Commits
rG440e936c47f5: Revert "sanitizers: increase .clang-format columns to 100"
Summary

This reverts commit 5d1df6d220f1d6f726d9643848679d781750db64.

There is a strong objection to this change:
https://reviews.llvm.org/D106436#2905618

Diff Detail

Unit TestsFailed

TimeTest
270 msx64 debian > LLVM.CodeGen/AMDGPU::annotate-kernel-features-hsa-call.ll
150 msx64 debian > LLVM.CodeGen/AMDGPU::annotate-kernel-features-hsa.ll
70 msx64 debian > LLVM.CodeGen/AMDGPU::direct-indirect-call.ll
80 msx64 debian > LLVM.CodeGen/AMDGPU::duplicate-attribute-indirect.ll
70 msx64 debian > LLVM.CodeGen/AMDGPU::simple-indirect-call.ll
View Full Test Results (10 Failed)

Event Timeline

dvyukov requested review of this revision.Jul 26 2021, 10:04 PM
dvyukov created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJul 26 2021, 10:04 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
dvyukov mentioned this in D106436: sanitizers: increase .clang-format columns to 100.Jul 26 2021, 10:05 PM
Harbormaster completed remote builds in B116344: Diff 361902.Jul 26 2021, 10:40 PM
dblaikie accepted this revision.Jul 27 2021, 5:28 AM

Approved (though usually reverts like this (especially of your own patches, shortly after originally committed, etc) can be submitted without pre-commit review, if you like) - happy if you'd like to wait a few days for other discussion, though.

This revision is now accepted and ready to land.Jul 27 2021, 5:28 AM
dvyukov added a comment.Jul 27 2021, 5:51 AM
In D106847#2907052, @dblaikie wrote:

happy if you'd like to wait a few days for other discussion, though.

What is the other discussion?

MaskRay added a comment.Jul 27 2021, 9:32 AM

Perhaps it means a discussion on llvm-dev.

dblaikie added a comment.Jul 27 2021, 2:17 PM
In D106847#2907106, @dvyukov wrote:
In D106847#2907052, @dblaikie wrote:

happy if you'd like to wait a few days for other discussion, though.

What is the other discussion?

Oh, I just meant if you wanted to wait until/if other people had feedback on the patch, that'd be OK. But otherwise you can go ahead and commit the revert with my approval.

MaskRay added a comment.Jul 27 2021, 2:46 PM

I think a discussion needs to be made before the revert is commited (which can cause churn by itself).

FWIW customizing ColumnLimit: isn't uncommon in llvm-project:

% rg ColumnLimit: -g .clang-format -l | grep -v compiler-rt
pstl/.clang-format
libcxx/.clang-format
clang-tools-extra/test/.clang-format
clang/test/.clang-format
llvm/test/.clang-format
lldb/test/Shell/.clang-format

If we drop tests, pstl and libcxx diverge from the llvm style.

vitalybuka resigned from this revision.Jul 27 2021, 3:32 PM
xgupta added a subscriber: xgupta.EditedJul 27 2021, 4:50 PM
In D106847#2908664, @MaskRay wrote:

I think a discussion needs to be made before the revert is commited (which can cause churn by itself).

FWIW customizing ColumnLimit: isn't uncommon in llvm-project:

% rg ColumnLimit: -g .clang-format -l | grep -v compiler-rt
pstl/.clang-format
libcxx/.clang-format
clang-tools-extra/test/.clang-format
clang/test/.clang-format
llvm/test/.clang-format
lldb/test/Shell/.clang-format

If we drop tests, pstl and libcxx diverge from the llvm style.

Coding standards for column width are about source code(.c, .cpp, .h), so we can drop tests, not many ( i think), run clang-format on them.

pstl comes from a single commit - https://github.com/intel/parallelstl.
in https://reviews.llvm.org/D55889#1336439 as said jfb, on RFC, people are agree to committed the patch as-is. No attention was paid on ColumnLimit.

libcxx change .clang-format standard recently(15 April 2021) in https://reviews.llvm.org/D99691#2690379. And they are always open to tweak it.

dvyukov added a comment.Jul 28 2021, 12:38 AM
In D106847#2908664, @MaskRay wrote:

I think a discussion needs to be made before the revert is commited (which can cause churn by itself).

You are getting me into a deadlock state, I cannot both revert and not revert this :)

This revision was landed with ongoing or failed builds.Jul 28 2021, 12:40 AM
Closed by commit rG440e936c47f5: Revert "sanitizers: increase .clang-format columns to 100" (authored by dvyukov). · Explain Why
This revision was automatically updated to reflect the committed changes.
dvyukov added a commit: rG440e936c47f5: Revert "sanitizers: increase .clang-format columns to 100".

Revision Contents

PathSize
compiler-rt/
lib/
asan/
1 line
dfsan/
1 line
hwasan/
1 line
interception/
1 line
lsan/
1 line
msan/
1 line
safestack/
1 line
sanitizer_common/
1 line
tsan/
1 line

Diff 361902

compiler-rt/lib/asan/.clang-format

BasedOnStyle: Google BasedOnStyle: Google
ColumnLimit: 100
AllowShortIfStatementsOnASingleLine: false AllowShortIfStatementsOnASingleLine: false
IndentPPDirectives: AfterHash IndentPPDirectives: AfterHash

compiler-rt/lib/dfsan/.clang-format

BasedOnStyle: Google BasedOnStyle: Google
ColumnLimit: 100
AllowShortIfStatementsOnASingleLine: false AllowShortIfStatementsOnASingleLine: false
IndentPPDirectives: AfterHash IndentPPDirectives: AfterHash

compiler-rt/lib/hwasan/.clang-format

BasedOnStyle: Google BasedOnStyle: Google
ColumnLimit: 100
AllowShortIfStatementsOnASingleLine: false AllowShortIfStatementsOnASingleLine: false
IndentPPDirectives: AfterHash IndentPPDirectives: AfterHash

compiler-rt/lib/interception/.clang-format

BasedOnStyle: Google BasedOnStyle: Google
ColumnLimit: 100
AllowShortIfStatementsOnASingleLine: false AllowShortIfStatementsOnASingleLine: false
IndentPPDirectives: AfterHash IndentPPDirectives: AfterHash

compiler-rt/lib/lsan/.clang-format

BasedOnStyle: Google BasedOnStyle: Google
ColumnLimit: 100
AllowShortIfStatementsOnASingleLine: false AllowShortIfStatementsOnASingleLine: false
IndentPPDirectives: AfterHash IndentPPDirectives: AfterHash

compiler-rt/lib/msan/.clang-format

BasedOnStyle: Google BasedOnStyle: Google
ColumnLimit: 100
AllowShortIfStatementsOnASingleLine: false AllowShortIfStatementsOnASingleLine: false
IndentPPDirectives: AfterHash IndentPPDirectives: AfterHash

compiler-rt/lib/safestack/.clang-format

BasedOnStyle: Google BasedOnStyle: Google
ColumnLimit: 100
AllowShortIfStatementsOnASingleLine: false AllowShortIfStatementsOnASingleLine: false
IndentPPDirectives: AfterHash IndentPPDirectives: AfterHash

compiler-rt/lib/sanitizer_common/.clang-format

BasedOnStyle: Google BasedOnStyle: Google
ColumnLimit: 100
AllowShortIfStatementsOnASingleLine: false AllowShortIfStatementsOnASingleLine: false
IndentPPDirectives: AfterHash IndentPPDirectives: AfterHash

compiler-rt/lib/tsan/.clang-format

BasedOnStyle: Google BasedOnStyle: Google
ColumnLimit: 100
AllowShortIfStatementsOnASingleLine: false AllowShortIfStatementsOnASingleLine: false
IndentPPDirectives: AfterHash IndentPPDirectives: AfterHash