[PATCH] D148835: [clang] removes trailing whitespace

[Christopher Di Bella via Phabricator via cfe-commits]

cjdb added a comment.

In D148835#4288151 , @erichkeane 
wrote:

> In D148835#4288148 , @cjdb wrote:
>
>> In D148835#4286924 , 
>> @aaron.ballman wrote:
>>
>>> In D148835#4286905 , @erichkeane 
>>> wrote:
>>>
 In D148835#4284871 , @cjdb wrote:

> I've had some very good input about why this probably shouldn't go ahead: 
> git history erasure :')

 While that is a common criticism, if we're going to do this at all, we 
 should do it in a single patch, as we can use --ignore-rev: 
 https://akrabat.com/ignoring-revisions-with-git-blame/

 This is a pretty common thing to do, and I don't think it should prevent 
 us from doing this, which I think is the 'greater good' here.  The 
 alternative is to continue having a bunch of unrelated patches piece-meal 
 'erase' git history as people accidentally fix these.
>>>
>>> We already ignore blame revisions like this today: 
>>> https://github.com/llvm/llvm-project/blob/main/.git-blame-ignore-revs
>>
>> Oh cool, I had no idea this was possible :)
>>
>>> However, I'd ask that we hold off on this change unless we have some 
>>> tooling in place that rejects new additions of trailing whitespace, 
>>> otherwise we're going to end up in this exact same situation again in 
>>> another week or two. When CI starts failing on patches that insert *new* 
>>> trailing whitespace, then I'm all for this change because we can fix it 
>>> once and hopefully keep it fixed.
>>
>> Since Clang doesn't have pre-commit CI, how can we meaningfully progress 
>> here?
>
> Phab at least has Pre-commit CI, i would expect that to be sufficient? It 
> would at least cut down accidential whitespace by orders of magnitude.

Right, but Phab's precommit CI doesn't seem to be a blocker for merging, and 
often has false negatives, so I expect stuff to slip through :/

In D148835#4288248 , @sbc100 wrote:

> In D148835#4288151 , @erichkeane 
> wrote:
>
>> In D148835#4288148 , @cjdb wrote:
>>
>>> In D148835#4286924 , 
>>> @aaron.ballman wrote:
>>>
 In D148835#4286905 , @erichkeane 
 wrote:

> In D148835#4284871 , @cjdb 
> wrote:
>
>> I've had some very good input about why this probably shouldn't go 
>> ahead: git history erasure :')
>
> While that is a common criticism, if we're going to do this at all, we 
> should do it in a single patch, as we can use --ignore-rev: 
> https://akrabat.com/ignoring-revisions-with-git-blame/
>
> This is a pretty common thing to do, and I don't think it should prevent 
> us from doing this, which I think is the 'greater good' here.  The 
> alternative is to continue having a bunch of unrelated patches piece-meal 
> 'erase' git history as people accidentally fix these.

 We already ignore blame revisions like this today: 
 https://github.com/llvm/llvm-project/blob/main/.git-blame-ignore-revs
>>>
>>> Oh cool, I had no idea this was possible :)
>>>
 However, I'd ask that we hold off on this change unless we have some 
 tooling in place that rejects new additions of trailing whitespace, 
 otherwise we're going to end up in this exact same situation again in 
 another week or two. When CI starts failing on patches that insert *new* 
 trailing whitespace, then I'm all for this change because we can fix it 
 once and hopefully keep it fixed.
>>>
>>> Since Clang doesn't have pre-commit CI, how can we meaningfully progress 
>>> here?
>>
>> Phab at least has Pre-commit CI, i would expect that to be sufficient? It 
>> would at least cut down accidential whitespace by orders of magnitude.
>
> Yes, isn't this already covered by the existing use of `clang-format` on 
> upload to phabricator (`arc diff`) ?At least for me it always gives me 
> warnings if I try to upload anything that doesn't match clang-format.

There are folks who have historically uploaded using the web UI specifically to 
ignore clang-format's suggestions, and `arc diff --nolint` will skip the 
formatting stage.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148835

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148835: [clang] removes trailing whitespace

[Sam Clegg via Phabricator via cfe-commits]

sbc100 added a comment.

In D148835#4288151 , @erichkeane 
wrote:

> In D148835#4288148 , @cjdb wrote:
>
>> In D148835#4286924 , 
>> @aaron.ballman wrote:
>>
>>> In D148835#4286905 , @erichkeane 
>>> wrote:
>>>
 In D148835#4284871 , @cjdb wrote:

> I've had some very good input about why this probably shouldn't go ahead: 
> git history erasure :')

 While that is a common criticism, if we're going to do this at all, we 
 should do it in a single patch, as we can use --ignore-rev: 
 https://akrabat.com/ignoring-revisions-with-git-blame/

 This is a pretty common thing to do, and I don't think it should prevent 
 us from doing this, which I think is the 'greater good' here.  The 
 alternative is to continue having a bunch of unrelated patches piece-meal 
 'erase' git history as people accidentally fix these.
>>>
>>> We already ignore blame revisions like this today: 
>>> https://github.com/llvm/llvm-project/blob/main/.git-blame-ignore-revs
>>
>> Oh cool, I had no idea this was possible :)
>>
>>> However, I'd ask that we hold off on this change unless we have some 
>>> tooling in place that rejects new additions of trailing whitespace, 
>>> otherwise we're going to end up in this exact same situation again in 
>>> another week or two. When CI starts failing on patches that insert *new* 
>>> trailing whitespace, then I'm all for this change because we can fix it 
>>> once and hopefully keep it fixed.
>>
>> Since Clang doesn't have pre-commit CI, how can we meaningfully progress 
>> here?
>
> Phab at least has Pre-commit CI, i would expect that to be sufficient? It 
> would at least cut down accidential whitespace by orders of magnitude.

Yes, isn't this already covered by the existing use of `clang-format` on upload 
to phabricator (`arc diff`) ?At least for me it always gives me warnings if 
I try to upload anything that doesn't match clang-format.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148835

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148835: [clang] removes trailing whitespace

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D148835#4288148 , @cjdb wrote:

> In D148835#4286924 , @aaron.ballman 
> wrote:
>
>> In D148835#4286905 , @erichkeane 
>> wrote:
>>
>>> In D148835#4284871 , @cjdb wrote:
>>>
 I've had some very good input about why this probably shouldn't go ahead: 
 git history erasure :')
>>>
>>> While that is a common criticism, if we're going to do this at all, we 
>>> should do it in a single patch, as we can use --ignore-rev: 
>>> https://akrabat.com/ignoring-revisions-with-git-blame/
>>>
>>> This is a pretty common thing to do, and I don't think it should prevent us 
>>> from doing this, which I think is the 'greater good' here.  The alternative 
>>> is to continue having a bunch of unrelated patches piece-meal 'erase' git 
>>> history as people accidentally fix these.
>>
>> We already ignore blame revisions like this today: 
>> https://github.com/llvm/llvm-project/blob/main/.git-blame-ignore-revs
>
> Oh cool, I had no idea this was possible :)
>
>> However, I'd ask that we hold off on this change unless we have some tooling 
>> in place that rejects new additions of trailing whitespace, otherwise we're 
>> going to end up in this exact same situation again in another week or two. 
>> When CI starts failing on patches that insert *new* trailing whitespace, 
>> then I'm all for this change because we can fix it once and hopefully keep 
>> it fixed.
>
> Since Clang doesn't have pre-commit CI, how can we meaningfully progress here?

Phab at least has Pre-commit CI, i would expect that to be sufficient? It would 
at least cut down accidential whitespace by orders of magnitude.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148835

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148835: [clang] removes trailing whitespace

[Christopher Di Bella via Phabricator via cfe-commits]

cjdb added a comment.

In D148835#4286924 , @aaron.ballman 
wrote:

> In D148835#4286905 , @erichkeane 
> wrote:
>
>> In D148835#4284871 , @cjdb wrote:
>>
>>> I've had some very good input about why this probably shouldn't go ahead: 
>>> git history erasure :')
>>
>> While that is a common criticism, if we're going to do this at all, we 
>> should do it in a single patch, as we can use --ignore-rev: 
>> https://akrabat.com/ignoring-revisions-with-git-blame/
>>
>> This is a pretty common thing to do, and I don't think it should prevent us 
>> from doing this, which I think is the 'greater good' here.  The alternative 
>> is to continue having a bunch of unrelated patches piece-meal 'erase' git 
>> history as people accidentally fix these.
>
> We already ignore blame revisions like this today: 
> https://github.com/llvm/llvm-project/blob/main/.git-blame-ignore-revs

Oh cool, I had no idea this was possible :)

> However, I'd ask that we hold off on this change unless we have some tooling 
> in place that rejects new additions of trailing whitespace, otherwise we're 
> going to end up in this exact same situation again in another week or two. 
> When CI starts failing on patches that insert *new* trailing whitespace, then 
> I'm all for this change because we can fix it once and hopefully keep it 
> fixed.

Since Clang doesn't have pre-commit CI, how can we meaningfully progress here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148835

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148835: [clang] removes trailing whitespace

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D148835#4286905 , @erichkeane 
wrote:

> In D148835#4284871 , @cjdb wrote:
>
>> I've had some very good input about why this probably shouldn't go ahead: 
>> git history erasure :')
>
> While that is a common criticism, if we're going to do this at all, we should 
> do it in a single patch, as we can use --ignore-rev: 
> https://akrabat.com/ignoring-revisions-with-git-blame/
>
> This is a pretty common thing to do, and I don't think it should prevent us 
> from doing this, which I think is the 'greater good' here.  The alternative 
> is to continue having a bunch of unrelated patches piece-meal 'erase' git 
> history as people accidentally fix these.

We already ignore blame revisions like this today: 
https://github.com/llvm/llvm-project/blob/main/.git-blame-ignore-revs

However, I'd ask that we hold off on this change unless we have some tooling in 
place that rejects new additions of trailing whitespace, otherwise we're going 
to end up in this exact same situation again in another week or two. When CI 
starts failing on patches that insert *new* trailing whitespace, then I'm all 
for this change because we can fix it once and hopefully keep it fixed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148835

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148835: [clang] removes trailing whitespace

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D148835#4284871 , @cjdb wrote:

> I've had some very good input about why this probably shouldn't go ahead: git 
> history erasure :')

While that is a common criticism, if we're going to do this at all, we should 
do it in a single patch, as we can use --ignore-rev: 
https://akrabat.com/ignoring-revisions-with-git-blame/

This is a pretty common thing to do, and I don't think it should prevent us 
from doing this, which I think is the 'greater good' here.  The alternative is 
to continue having a bunch of unrelated patches piece-meal 'erase' git history 
as people accidentally fix these.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148835

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148835: [clang] removes trailing whitespace

[Christopher Di Bella via Phabricator via cfe-commits]

cjdb abandoned this revision.
cjdb added a comment.

I've had some very good input about why this probably shouldn't go ahead: git 
history erasure :')


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148835

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148835: [clang] removes trailing whitespace

[Christopher Di Bella via Phabricator via cfe-commits]

cjdb created this revision.
cjdb added reviewers: aaron.ballman, shafik, erichkeane, dblaikie.
Herald added subscribers: luke, steakhal, bzcheeseman, kosarev, pmatos, asb, 
ormris, frasercrmck, jdoerfert, martong, luismarques, apazos, sameer.abuasal, 
pengfei, s.egerton, Jim, jocewei, PkmX, arphaman, the_o, brucehoult, 
MartinMosbeck, rogfer01, steven_wu, atanasyan, edward-jones, zzheng, jrtc27, 
niosHD, sabuasal, simoncook, johnrusso, rbar, kbarton, hiraditya, krytarowski, 
whisperity, sbc100, jvesely, nemanjai, dschuff.
Herald added a reviewer: paulkirth.
Herald added a reviewer: NoQ.
Herald added a reviewer: ributzka.
Herald added a project: All.
Herald added a comment.
cjdb requested review of this revision.
Herald added subscribers: cfe-commits, jplehr, pcwang-thead, sstefan1, MaskRay, 
aheejin.
Herald added a reviewer: jdoerfert.
Herald added a reviewer: dang.
Herald added a project: clang.

NOTE: Clang-Format Team Automated Review Comment

Your review contains a change to ClangFormatStyleOptions.rst but not a change 
to clang/include/clang/Format/Format.h

ClangFormatStyleOptions.rst is auto generated from Format.h via 
clang/docs/tools/dump_format_style.py,  please run this to regenerate the .rst

You can validate that the rst is valid by running.

  ./docs/tools/dump_format_style.py
  mkdir -p html
  /usr/bin/sphinx-build -n ./docs ./html


Lots of files have trailing whitespace characters, which get removed by
many text editors upon save. I've run `sed -i 's/\s*$//g'` on all files
in `clang` except for clang-format, and manually reverted anything that
causes `ninja check-clang check-clang-utils` to fail. This ensures that
commits of substance don't accrue bogus diffs and commentary requesting
they be reverted.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D148835

Files:
  clang/ModuleInfo.txt
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ClangLinkerWrapper.rst
  clang/docs/ClangOffloadPackager.rst
  clang/docs/ClangTransformerTutorial.rst
  clang/docs/DebuggingCoroutines.rst
  clang/docs/ExternalClangExamples.rst
  clang/docs/JSONCompilationDatabase.rst
  clang/docs/MisExpect.rst
  clang/docs/ReleaseNotes.rst
  clang/docs/StandardCPlusPlusModules.rst
  clang/docs/UndefinedBehaviorSanitizer.rst
  clang/docs/analyzer/checkers.rst
  clang/docs/analyzer/developer-docs/DebugChecks.rst
  clang/docs/tools/dump_ast_matchers.py
  clang/examples/PrintFunctionNames/PrintFunctionNames.cpp
  clang/include/clang/AST/DeclCXX.h
  clang/include/clang/AST/DeclObjC.h
  clang/include/clang/AST/TypeProperties.td
  clang/include/clang/Analysis/AnalysisDeclContext.h
  clang/include/clang/Analysis/CallGraph.h
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Basic/OpenCLExtensions.def
  clang/include/clang/Basic/TypeNodes.td
  clang/include/clang/Basic/riscv_vector.td
  clang/include/clang/Sema/CMakeLists.txt
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/Analysis/FlowSensitive/CMakeLists.txt
  clang/lib/Analysis/UninitializedValues.cpp
  clang/lib/Analysis/UnsafeBufferUsage.cpp
  clang/lib/Basic/Targets/PPC.cpp
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/NSAutoreleasePoolChecker.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/lib/Tooling/DumpTool/ClangSrcLocDump.cpp
  clang/test/ARCMT/Common.h
  clang/test/ARCMT/Inputs/module.map
  clang/test/ARCMT/atautorelease-check.m
  clang/test/ARCMT/checking.m
  clang/test/ARCMT/cxx-checking.mm
  clang/test/ARCMT/designated-init-in-header/file2.m.in
  clang/test/ARCMT/designated-init-in-header/file2.m.in.result
  clang/test/ARCMT/no-canceling-bridge-to-bridge-cast.m
  clang/test/ARCMT/objcmt-arc-cf-annotations.m
  clang/test/ARCMT/objcmt-arc-cf-annotations.m.result
  clang/test/ARCMT/objcmt-atomic-property.m
  clang/test/ARCMT/objcmt-atomic-property.m.result
  clang/test/ARCMT/objcmt-boxing.m
  clang/test/ARCMT/objcmt-boxing.m.result
  clang/test/ARCMT/objcmt-migrate-all.m
  clang/test/ARCMT/objcmt-migrate-all.m.result
  clang/test/ARCMT/objcmt-ns-macros.m
  clang/test/ARCMT/objcmt-ns-macros.m.result
  clang/test/ARCMT/objcmt-ns-nonatomic-iosonly.m
  clang/test/ARCMT/objcmt-ns-nonatomic-iosonly.m.result
  clang/test/ARCMT/objcmt-ns-returns-inner-pointer.m
  clang/test/ARCMT/objcmt-ns-returns-inner-pointer.m.result
  clang/test/ARCMT/objcmt-numeric-literals.m
  clang/test/ARCMT/objcmt-numeric-literals.m.result
  clang/test/ARCMT/objcmt-property-dot-syntax.m
  clang/test/ARCMT/objcmt-property-dot-syntax.m.result
  clang/test/ARCMT/objcmt-property.m
  clang/test/ARCMT/objcmt-property.m.result
  clang/test/ARCMT/objcmt-protocol-conformance.m