[Impala-ASF-CR] Match .clang-format more closely to actual practice.

[Jim Apple (Code Review)]

Jim Apple has submitted this change and it was merged.

Change subject: Match .clang-format more closely to actual practice.
..


Match .clang-format more closely to actual practice.

In order to attempt to get code like

double VeryLongFunctionNames(double x1, double x2, double x3,
double x4) {
  return 1.0;
}

rather than

double VeryLongFunctionNames(
double x1, double x2, double x3, double x4) {
  return 1.0;
}

I wrote a small set of programs to infer which .clang-format params
fit the current Impala codebase most closely; this patch is the
result.

This patch is the best the inferencer found (while maintaining certain
enforced parameters, like 90-character lines). It is about 10% closer
to Impala's current code base than the .clang-format that is checked
in at the moment, as measured by number of lines in the diff.

Change-Id: Iccaec6c1673c3e08d2c39200b0c84437af629aed
Reviewed-on: http://gerrit.cloudera.org:8080/4590
Reviewed-by: Jim Apple 
Tested-by: Jim Apple 
---
M .clang-format
1 file changed, 35 insertions(+), 14 deletions(-)

Approvals:
  Jim Apple: Looks good to me, approved; Verified



-- 
To view, visit http://gerrit.cloudera.org:8080/4590
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Iccaec6c1673c3e08d2c39200b0c84437af629aed
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] Match .clang-format more closely to actual practice.

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: Match .clang-format more closely to actual practice.
..


Patch Set 3: Verified+1

Can't break tests with this change, so +1 verify manually

-- 
To view, visit http://gerrit.cloudera.org:8080/4590
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iccaec6c1673c3e08d2c39200b0c84437af629aed
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] Match .clang-format more closely to actual practice.

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: Match .clang-format more closely to actual practice.
..


Patch Set 3: Code-Review+2

rebase, carry +1 into +2

-- 
To view, visit http://gerrit.cloudera.org:8080/4590
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iccaec6c1673c3e08d2c39200b0c84437af629aed
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] Match .clang-format more closely to actual practice.

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: Match .clang-format more closely to actual practice.
..


Patch Set 2:

> > I'm not sure I understand why a person might want to run the
 > inferencer again, even if the relationship between config files and
 > the induced changes are sometimes confusing.
 > 
 > If I don't understand how to get the results I want (because the
 > formatter has become a black box), the only recourse left to me is
 > to ask a machine to do it for me - just like has happened here.

I think I understand what you mean. Unfortunately, I think that's an issue with 
the number of clang-format options. I think we will face that issue with or 
without inference.

-- 
To view, visit http://gerrit.cloudera.org:8080/4590
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iccaec6c1673c3e08d2c39200b0c84437af629aed
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] Match .clang-format more closely to actual practice.

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

Change subject: Match .clang-format more closely to actual practice.
..


Patch Set 2:

Do you understand the relationship between the resulting .clang-format file and 
the changes in formatting? I would rather not get into a situation where I have 
to re-run an inferencer to make small changes to the format file because we 
don't understand the dependency of the outputs on the inputs. 

FWIW I'm quite happy with the current formatter, but willing to cede to others 
if they feel strongly.

-- 
To view, visit http://gerrit.cloudera.org:8080/4590
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iccaec6c1673c3e08d2c39200b0c84437af629aed
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] Match .clang-format more closely to actual practice.

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: Match .clang-format more closely to actual practice.
..


Patch Set 2: Code-Review+1

-- 
To view, visit http://gerrit.cloudera.org:8080/4590
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iccaec6c1673c3e08d2c39200b0c84437af629aed
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] Match .clang-format more closely to actual practice.

[Jim Apple (Code Review)]

Jim Apple has uploaded a new patch set (#2).

Change subject: Match .clang-format more closely to actual practice.
..

Match .clang-format more closely to actual practice.

In order to attempt to get code like

double VeryLongFunctionNames(double x1, double x2, double x3,
double x4) {
  return 1.0;
}

rather than

double VeryLongFunctionNames(
double x1, double x2, double x3, double x4) {
  return 1.0;
}

I wrote a small set of programs to infer which .clang-format params
fit the current Impala codebase most closely; this patch is the
result.

This patch is the best the inferencer found (while maintaining certain
enforced parameters, like 90-character lines). It is about 10% closer
to Impala's current code base than the .clang-format that is checked
in at the moment, as measured by number of lines in the diff.

Change-Id: Iccaec6c1673c3e08d2c39200b0c84437af629aed
---
M .clang-format
1 file changed, 35 insertions(+), 14 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/4590/2
-- 
To view, visit http://gerrit.cloudera.org:8080/4590
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iccaec6c1673c3e08d2c39200b0c84437af629aed
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] Match .clang-format more closely to actual practice.

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: Match .clang-format more closely to actual practice.
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4590/1/.clang-format
File .clang-format:

Line 6
Putting short if statements on a single line is something we've definitely 
encouraged. Does this make the diff smaller?


Line 34: DerivePointerAlignment: false
This doesn't seem relevant for Java.


-- 
To view, visit http://gerrit.cloudera.org:8080/4590
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iccaec6c1673c3e08d2c39200b0c84437af629aed
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes