This is an archive of the discontinued Mercurial Phabricator instance.

mpatch: reformat function prototypes with clang-format
ClosedPublic

Authored by durin42 on Oct 12 2017, 11:09 AM.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

durin42 created this revision.Oct 12 2017, 11:09 AM
pulkit accepted this revision.Oct 12 2017, 1:47 PM
yuja requested changes to this revision.Oct 13 2017, 9:19 AM
yuja added a subscriber: yuja.

Seemed fine, but test-check-code.t says no.

+  mercurial/mpatch.c:268:
+   >             ssize_t start, ssize_t end)
+   don't use spaces to indent
+  mercurial/mpatch.h:24:
+   >             ssize_t start, ssize_t end);
+   don't use spaces to indent
This revision now requires changes to proceed.Oct 13 2017, 9:19 AM

I thought it was string that check-code doesn't fire on D1030 but does fire here.

It turns out that the check-code only finds spaces at the start of the line. So is you start with a tab and then spaces, that's fine. But if you start with spaces you trip up against test-check-code. In D1030, the re-indented line start with a tab, whereas here the re-indented line is top-level so it starts with spaces.

I, for one, am -1 on tabs but that's not a battle worth fighting probably. However, we can probably come up with a smarter test-check-code. If we're moving towards clang-format, it seems that the test-code should be checking that and not arbitrarily for spaces at the start of a line.

In D1028#17513, @yuja wrote:

Seemed fine, but test-check-code.t says no.

+  mercurial/mpatch.c:268:
+   >             ssize_t start, ssize_t end)
+   don't use spaces to indent
+  mercurial/mpatch.h:24:
+   >             ssize_t start, ssize_t end);
+   don't use spaces to indent
In D1028#17513, @yuja wrote:

Seemed fine, but test-check-code.t says no.

+  mercurial/mpatch.c:268:
+   >             ssize_t start, ssize_t end)
+   don't use spaces to indent
+  mercurial/mpatch.h:24:
+   >             ssize_t start, ssize_t end);
+   don't use spaces to indent

Ah. Okay. Since everyone at the sprint seemed loosely in favor of clang-format, how about this: I'll get the remaining trivial fixes from clang-format in, then land a test-check-clang-format.t with an extensive blacklist, then remove the check-code rules that are offended by clang-format as the series exposes them. That way we own less checking, and the formatting is done by a robot.

That work?

I, for one, am -1 on tabs but that's not a battle worth fighting probably. However, we can probably come up with a smarter test-check-code. If we're moving towards clang-format, it seems that the test-code should be checking that and not arbitrarily for spaces at the start of a line.

I'm going to forge ahead with the coding style we nominally have in the codebase today, and we can consider changes once we're done with the initial move to the formatter. Sound good?

(I don't feel especially strongly, though my personal preference is still tabs-for-indentation spaces-for-alignment, which clang-format happily can do. I just have better things to argue about, which is why clang-format is great, especially if you get editor integration set up.)

durin42 updated this revision to Diff 2823.Oct 16 2017, 11:53 AM
This revision was automatically updated to reflect the committed changes.