Page MenuHomePhabricator

[clang-format] BeforeHash added to IndentPPDirectives
ClosedPublic

Authored by to-miz on Sep 16 2018, 11:14 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

to-miz created this revision.Sep 16 2018, 11:14 AM
to-miz updated this revision to Diff 165700.Sep 16 2018, 3:35 PM

PPBranchLevel can be negative, while Line->Level is unsigned. Added check to ensure that PPBranchLevel is positive before adding to Line->Level.

krasimir requested changes to this revision.Oct 18 2018, 1:58 PM

As per https://clang.llvm.org/docs/ClangFormatStyleOptions.html#adding-additional-style-options, could you please provide more info about the need for this option first?

This revision now requires changes to proceed.Oct 18 2018, 1:58 PM
bjehlert removed a subscriber: bjehlert.Nov 1 2018, 3:58 PM
ufna added a subscriber: ufna.Jan 17 2019, 6:59 AM

@krasimir BeforeHash style is widely used for Unreal Engine 4 based projects development. I was looking for this option too.

geleji added a subscriber: geleji.Jan 30 2019, 1:08 AM

I would find this useful too.

mnussbaum added a subscriber: mnussbaum.

I understand that adding new options adds costs to the development of clang-format, but there was a feature request and there is interest for this option.
As for the cost to the development, there already exists PPDIS_AfterHash. All I did was make the changes introduced in that patch a bit more general and removed the hard coded assumption that there are no indents before a hash at a couple of places.
I would argue the cost of development of this new option is roughly the same as it is with the existence of PPDIS_AfterHash itself. PPDIS_BeforeHash seems like a natural extension of the changes introduced by PPDIS_AfterHash.

Could you add some text into the docs/ReleaseNotes.rst to say you are adding a new option

take a look at https://clang.llvm.org/extra/ReleaseNotes.html#improvements-to-clang-tidy for an example

MyDeveloperDay marked an inline comment as done.Feb 21 2019, 2:03 AM
MyDeveloperDay added inline comments.
lib/Format/UnwrappedLineFormatter.cpp
15 ↗(On Diff #165700)

Nice! its the irony of clang-format code is that its not clang formatted!!

MyDeveloperDay edited the summary of this revision. (Show Details)Feb 21 2019, 2:37 AM

While we wait for code review I've landed this currently unaccepted clang-format revision into https://github.com/mydeveloperday/clang-experimental/releases for those wishing to try/test or provide feedback

to-miz updated this revision to Diff 189048.Mar 2 2019, 7:26 AM

Added an entry to ReleaseNotes.rst about the new option.
This diff doesn't contain unrelated formatting changes due to running clang-format on some source files that apparently weren't formatted before.

MyDeveloperDay accepted this revision.EditedMar 2 2019, 7:34 AM

Thank you for helping to address one of the many clang-format issues from bugzilla, I'm not the code owner here but this LGTM.

klimek accepted this revision.Mar 14 2019, 4:00 AM

Given this doesn't add an extra option (but only an extra enum) and is fairly straight forward, LG.

@to-miz do you have permission to commit?

I don't have commit access. It would be nice if someone could commit it in my place.

Could you do a rebase? I've tried to land it but there are too many merge conflicts currently.

Is it because the diff was made with the svn repository? It is for the newest revision.
Should I make a new diff with the git repo instead?
Or is it not because of the diff, but something in phabricator instead?
Sorry this is my first patch.

Is it because the diff was made with the svn repository? It is for the newest revision.
Should I make a new diff with the git repo instead?
Or is it not because of the diff, but something in phabricator instead?
Sorry this is my first patch.

No worries. It is just because you've made your last update at 2nd of march(assuming you've rebased before that, otherwise it might be as old as last september) and its 20th today, there are other people making changes, which simply conflicts with yours.

I checked out the git repository and applied the patch to head and reran all tests.
The diff produced by git is basically the same, should I upload the new diff?

I checked out the git repository and applied the patch to head and reran all tests.
The diff produced by git is basically the same, should I upload the new diff?

I think you might need a new diff, I tried to merge this and it rejected some of the patch

to-miz updated this revision to Diff 191540.Mar 20 2019, 11:09 AM

Made a new diff on the git repository, since the docs now recommend using git.
Rebased and created a new diff with git show HEAD -U999999.
The patch applies cleanly on windows after git reset --hard so I hope there shouldn't be any merge conflicts now.

This revision was not accepted when it landed; it landed in state Needs Review.Mar 20 2019, 1:48 PM
Closed by commit rL356613: [clang-format] BeforeHash added to IndentPPDirectives (authored by paulhoad, committed by ). · Explain Why
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 20 2019, 1:48 PM