[kudu-CR] Add .clang-format file
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16260 ) Change subject: Add .clang-format file .. Add .clang-format file This clang-format file seems to capture our existing style relatively well. I've been using it on my patches and only gotten a few complaints where the automatic formatting isn't quite what people expected, but I think the benefits of uniformity outweigh any particular aesthetic choices we might make. This patch updates the documentation to suggest running this on patches to modify only the changed lines using clang-format-diff. Change-Id: I41def9a77bd98bf09353fe9a7789756a1bff30c1 Reviewed-on: http://gerrit.cloudera.org:8080/16260 Reviewed-by: Attila Bukor Tested-by: Todd Lipcon --- A build-support/clang_format_diff.sh M docs/contributing.adoc A src/kudu/.clang-format 3 files changed, 94 insertions(+), 2 deletions(-) Approvals: Attila Bukor: Looks good to me, approved Todd Lipcon: Verified -- To view, visit http://gerrit.cloudera.org:8080/16260 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I41def9a77bd98bf09353fe9a7789756a1bff30c1 Gerrit-Change-Number: 16260 Gerrit-PatchSet: 3 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Add .clang-format file
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/16260 ) Change subject: Add .clang-format file .. Patch Set 2: Verified+1 Not waiting for Jenkins since there's no code change here. -- To view, visit http://gerrit.cloudera.org:8080/16260 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I41def9a77bd98bf09353fe9a7789756a1bff30c1 Gerrit-Change-Number: 16260 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 25 Aug 2020 22:29:28 + Gerrit-HasComments: No
[kudu-CR] Add .clang-format file
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/16260 ) Change subject: Add .clang-format file .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/16260 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I41def9a77bd98bf09353fe9a7789756a1bff30c1 Gerrit-Change-Number: 16260 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 25 Aug 2020 22:26:59 + Gerrit-HasComments: No
[kudu-CR] Add .clang-format file
Hello Alexey Serbin, Attila Bukor, Kudu Jenkins, Grant Henke, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16260 to look at the new patch set (#2). Change subject: Add .clang-format file .. Add .clang-format file This clang-format file seems to capture our existing style relatively well. I've been using it on my patches and only gotten a few complaints where the automatic formatting isn't quite what people expected, but I think the benefits of uniformity outweigh any particular aesthetic choices we might make. This patch updates the documentation to suggest running this on patches to modify only the changed lines using clang-format-diff. Change-Id: I41def9a77bd98bf09353fe9a7789756a1bff30c1 --- A build-support/clang_format_diff.sh M docs/contributing.adoc A src/kudu/.clang-format 3 files changed, 94 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/16260/2 -- To view, visit http://gerrit.cloudera.org:8080/16260 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I41def9a77bd98bf09353fe9a7789756a1bff30c1 Gerrit-Change-Number: 16260 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Add .clang-format file
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/16260 ) Change subject: Add .clang-format file .. Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/16260/1/docs/contributing.adoc File docs/contributing.adoc: http://gerrit.cloudera.org:8080/#/c/16260/1/docs/contributing.adoc@205 PS1, Line 205: git show -U0 | thirdparty/installed/uninstrumented/share/clang/clang-format-diff.py -i -p1 > Maybe mention this assumes clang-format is installed in $PATH and offer the I added a simple wrapper script in build-support so this is easier to deal with. http://gerrit.cloudera.org:8080/#/c/16260/1/src/kudu/.clang-format File src/kudu/.clang-format: http://gerrit.cloudera.org:8080/#/c/16260/1/src/kudu/.clang-format@16 PS1, Line 16: # under the License. > I guess adding a reference to the clang-format configuration guide might be Done http://gerrit.cloudera.org:8080/#/c/16260/1/src/kudu/.clang-format@21 PS1, Line 21: 98 > isn't our limit 100? Done http://gerrit.cloudera.org:8080/#/c/16260/1/src/kudu/.clang-format@18 PS1, Line 18: BasedOnStyle: Google : SortIncludes: false : DerivePointerAlignment: false : ColumnLimit: 98 : BinPackArguments: false : BinPackParameters: false > nit: it would be nice to add comments per-directive why it's there, if poss Done -- To view, visit http://gerrit.cloudera.org:8080/16260 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I41def9a77bd98bf09353fe9a7789756a1bff30c1 Gerrit-Change-Number: 16260 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 25 Aug 2020 22:11:09 + Gerrit-HasComments: Yes
[kudu-CR] Add .clang-format file
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/16260 ) Change subject: Add .clang-format file .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/16260/1/docs/contributing.adoc File docs/contributing.adoc: http://gerrit.cloudera.org:8080/#/c/16260/1/docs/contributing.adoc@205 PS1, Line 205: git show -U0 | thirdparty/installed/uninstrumented/share/clang/clang-format-diff.py -i -p1 > If clang-format isn't available in the PATH, this output an error. I think Maybe mention this assumes clang-format is installed in $PATH and offer the "-binary" version as an alternative. http://gerrit.cloudera.org:8080/#/c/16260/1/src/kudu/.clang-format File src/kudu/.clang-format: http://gerrit.cloudera.org:8080/#/c/16260/1/src/kudu/.clang-format@21 PS1, Line 21: 98 isn't our limit 100? -- To view, visit http://gerrit.cloudera.org:8080/16260 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I41def9a77bd98bf09353fe9a7789756a1bff30c1 Gerrit-Change-Number: 16260 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 31 Jul 2020 10:30:57 + Gerrit-HasComments: Yes
[kudu-CR] Add .clang-format file
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16260 ) Change subject: Add .clang-format file .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/16260/1/src/kudu/.clang-format File src/kudu/.clang-format: http://gerrit.cloudera.org:8080/#/c/16260/1/src/kudu/.clang-format@16 PS1, Line 16: # under the License. I guess adding a reference to the clang-format configuration guide might be handy: https://clang.llvm.org/docs/ClangFormatStyleOptions.html -- To view, visit http://gerrit.cloudera.org:8080/16260 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I41def9a77bd98bf09353fe9a7789756a1bff30c1 Gerrit-Change-Number: 16260 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 31 Jul 2020 01:26:00 + Gerrit-HasComments: Yes
[kudu-CR] Add .clang-format file
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16260 ) Change subject: Add .clang-format file .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/16260/1/docs/contributing.adoc File docs/contributing.adoc: http://gerrit.cloudera.org:8080/#/c/16260/1/docs/contributing.adoc@205 PS1, Line 205: git show -U0 | thirdparty/installed/uninstrumented/share/clang/clang-format-diff.py -i -p1 If clang-format isn't available in the PATH, this output an error. I think that specifying '-binary' option for the clang-format-diff.py would be more consistent in that regard: git show -U0 | thirdparty/installed/uninstrumented/share/clang/clang-format-diff.py -binary thirdparty/installed/uninstrumented/bin/clang-format -p1 -i http://gerrit.cloudera.org:8080/#/c/16260/1/src/kudu/.clang-format File src/kudu/.clang-format: http://gerrit.cloudera.org:8080/#/c/16260/1/src/kudu/.clang-format@18 PS1, Line 18: BasedOnStyle: Google : SortIncludes: false : DerivePointerAlignment: false : ColumnLimit: 98 : BinPackArguments: false : BinPackParameters: false nit: it would be nice to add comments per-directive why it's there, if possible. For example, I guess 'SortIncludes' is set to 'false' because it would conflict with IWYU sorting, right? -- To view, visit http://gerrit.cloudera.org:8080/16260 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I41def9a77bd98bf09353fe9a7789756a1bff30c1 Gerrit-Change-Number: 16260 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 30 Jul 2020 23:40:29 + Gerrit-HasComments: Yes
[kudu-CR] Add .clang-format file
Hello Alexey Serbin, Grant Henke, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/16260 to review the following change. Change subject: Add .clang-format file .. Add .clang-format file This clang-format file seems to capture our existing style relatively well. I've been using it on my patches and only gotten a few complaints where the automatic formatting isn't quite what people expected, but I think the benefits of uniformity outweigh any particular aesthetic choices we might make. This patch updates the documentation to suggest running this on patches to modify only the changed lines using clang-format-diff. Change-Id: I41def9a77bd98bf09353fe9a7789756a1bff30c1 --- M docs/contributing.adoc A src/kudu/.clang-format 2 files changed, 41 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/16260/1 -- To view, visit http://gerrit.cloudera.org:8080/16260 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I41def9a77bd98bf09353fe9a7789756a1bff30c1 Gerrit-Change-Number: 16260 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke
