Bug 230397 – It should be possible to format C++ code to project style with WebKit tools

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Kimmo Kinnunen]

It should be possible to format C++ code to project style with webkit tools

[Kimmo Kinnunen]

Created attachment 438463 [details]
Patch

[Kimmo Kinnunen]

Created attachment 438492 [details]
Patch

[Jonathan Bedard]

I think this is a really good idea, although I find it a bit curious that it's attached to webkit-patch rather than the style checker. Is this because attaching this to the style checker is more difficult since the style checker isn't the grab-bag of commands webkit-patch is?

For some context, webkit-patch is on the way out since it doesn't (and will not) support pull requests.

[Kimmo Kinnunen]

The primary reason from user interface perspective is that, for me, it does not make sense that there'd be 2 or 3 or 77 different scripts and commands for simple task of manipulating patches to get the in the tree. I understand that there might be 45 different steps to apply to the patch. I think my computer should just do those steps and get it done.

I understand the script to manipulate patches today is 'webkit-patch' and tomorrow it's 'git webkit'.

Today I'd prefer to use it with:
  webkit-patch format -g HEAD~....
  git commit --amend -a --no-edit
  webkit-patch upload -g HEAD

Tomorrow I'd prefer using it with:
  git webkit format
  git commit -a --amend --no-edit
  git webkit pull-request 

I can do the git-webkit part when that tool is useful (and it's according to what you think the tool should do, of course).



From implementation standpoint (e.g. where does the code go):

Problems with integrating this with the style checker is related to the overall abstraction that the checker provides.

Style checker doesn't have concept of "modify the working dir".
Consider:

check-webkit-style -g HEAD
 - Check the patch I'm working on and don't do anything to the working dir

check-webkit-style -g HEAD --format
 - Intuitively: first format patch and then check the style
 - Problem: what's the point of formatting the patch and then throwing it away?
 - Problem: if it writes to CWD, then the sense of abstraction of "check the patch" is lost.
 - Problem: it cannot write to "HEAD", it writes to CWD, which might already have different code and what do we then check?


Contrast with:

webkit-patch format -g HEAD
 - Format the working dir based on what's in the patch. (In general -g case it's a bit stupid, but it's better than nothing and works in a defined way)
 - There is less overload of abstraction about what the tool can do and how it operates.


Adding the concept of "modify the working dir" to the style checker. Contrast:

check-webkit-style -g HEAD
  - Check the patch in HEAD

check-webkit-style -g HEAD --format
  - Intuitively: first format patch in HEAD and then check the style of HEAD
  - However, it also implies that most possibly you want the cwd changed, so you actually get the results of the automatic formatting. Hence:
  - Intuitively2: format the CWD based on the patch and then check the style of CWD
  
So which is it:
  A) "Check the patch in the HEAD"
  B) "Check the CWD"
?


Style checker has extensive filtering of rules that aren't useful for the automatic formatting and are infeasible to implement. E.g it'd be confusing to do:
  check-webkit-style -g HEAD --format --filter -whitespace

Confusing because format wouldn't respect the filter. 

Not useful for formatting because there's very little point of providing capability of "personal mix and match of random rules" for formatting. E.g. if the goal is project-wide formatting, it's relatively small benefit to provide capability to do personal artistic formatting.

Here the comment talks about HEAD, but it could be anything of form HEAD~~~~....

Formatting of say HEAD~~~..HEAD~ should also work, but it's generally breaking the abstraction since, again, typically what you want to the formatting to do is to write the results somewhere, but in these cases the HEAD, index or working dir might have already changed. Still, the operation is defined, e.g. "change the cwd". 

If this was integrated with the style checker, the operation semantics would be strange: "without --format, check the patch, but with --format change the cwd and check ... something ?".

[Radar WebKit Bug Importer]

<rdar://problem/83492154>

[Jonathan Bedard]

Kimmo has convinced me that webkit-patch is the right place for this, at least at the moment. Hoping to get a chance to review this early next week.

[Sam Sneddon [:gsnedders]]

(In reply to Jonathan Bedard from comment #6)
> Kimmo has convinced me that webkit-patch is the right place for this, at
> least at the moment. Hoping to get a chance to review this early next week.

Kinda OT: I think we should follow the Mozilla and WPT approach of having a single entry-point for many of our scripts. At the moment webkit-patch functionally acts like this, hence this doesn't seem too bad to me.

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 438492 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Kimmo Kinnunen]

Created attachment 441853 [details]
Patch for landing

[EWS]

Committed r284520 (243263@main): <https://commits.webkit.org/243263@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 441853 [details].