I quickly checked whether changing the maxlens definition in presubmit_canned_checks.py in depot_tools to have 100 character limits for .m and .mm files, but the presubmit check still fails on .h files that are reformatted to use 100 characters line limit when the corresponding implementation file is an Objective-C file, so it is non-trivial to fix the presubmit check.

I have a CL (https://chromium-review.googlesource.com/c/434577/) that for change presubmit_canned_checks.py to check if there is a file named foo.m or foo.mm when checking foo.h for long lines and switch to use Objective-C line limits in that case (instead of C++ line limit). However, the CL do check if the file exists locally not whether it exists in the repository. I also do not know how clang-format would behave if you have foo.h and foo_mac.mm (or foo_ios.mm). Will it try to use 100 lines for foo.h or not, my change assume that it won't.

Another option would be to roll back the buildtools roll.

are you saying we should be using 100 or 80 for ObjC in Chromium?  it's not clear to me from your messages.

re: reverting the roll -- could we just do something like this for a short-term alleviation?

  ---
  Language: Objc
  ColumnLimit: 80
  ---
  Language: Cpp
  ColumnLimit: 80

in src/.clang-format?  For the longer term, maybe we could just change Chromium defaults[1] for FormatStyle::LK_ObjC and do a new roll?  and maybe start using .m or some other way to differentiate Objective C headers?

[1] https://github.com/llvm-mirror/clang/blob/HEAD/lib/Format/Format.cpp#L641

Language: ObjC*

if the change is that .mm are now interpreted as Language: ObjC, we may only need the first language block in my suggestion in comment 2

I was trying to say that:

1. Objective-C style guide recommends using 100 character line limit,
2. presubmit check enforce 80 character line limit for .m, .mm and .h,
3. clang-format roll changed the line limit of Objective-C files and headers from 80 to 100, thus breaking the presubmit for all iOS developers.

My suggestion was that we should fix presubmit check to respect what both the style guide and the auto formatter wants as line length limit for Objective-C source file and headers.

Regarding which extension to use for Objective-C header, we cannot use .m as this is used for Objective-C source file while .mm is used for Objective-C++ files (correspond to .c for C files and .cc for C++ files). Using anything besides .h will confuse our tooling (e.g. Xcode expects the header files to be named .h).

BTW, as src/ios/ requires code to be formatted with clang-format, and is mostly Objective-C++ code, this prevents anyone from landing code on iOS (as clang-format uses 100 character line limit, and presubmit wants both the code to be formatted with clang-format and to use less than 80 characters line limit).

We tried suggestion in comment #2, but this impact java that is now formatted at 80 columns. See https://codereview.chromium.org/2664133003/. Reverting the buildtools roll.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9afca38d59c642849c76fab7e623e9f6df8926fe

commit 9afca38d59c642849c76fab7e623e9f6df8926fe
Author: sdefresne <sdefresne@chromium.org>
Date: Tue Jan 31 12:19:05 2017

Revert of Roll buildtools a7cc7a3e21..c302711306 (patchset #2 id:20001 of https://codereview.chromium.org/2659853002/ )

Reason for revert:
Breaks presubmit for Objective-C code formatting in src/ios (due to https://bugs.chromium.org/p/chromium/issues/detail?id=686650).

Original issue's description:
> Roll buildtools a7cc7a3e21..c302711306 to pick up new clang-format binaries:
>
>   c302711306 Update clang-format binaries and scripts for all platforms (take 2).
>   a2ace8b013 Revert "Update clang-format binaries and scripts for all platforms."
>   99df659c6e Update clang-format binaries and scripts for all platforms.
>   df89892622 Fix "Updating clang-format" wiki link
>
> R=thakis@chromium.org
> BUG= 567770 
>
> Review-Url: https://codereview.chromium.org/2659853002
> Cr-Commit-Position: refs/heads/master@{#446842}
> Committed: https://chromium.googlesource.com/chromium/src/+/c380d9c4c0fbde77570427d8d8b11a4759652bf2

TBR=thakis@chromium.org,dbeam@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 567770 , 686650 

Review-Url: https://codereview.chromium.org/2664983003
Cr-Commit-Position: refs/heads/master@{#447219}

[modify] https://crrev.com/9afca38d59c642849c76fab7e623e9f6df8926fe/DEPS

FWIW, I believe we could have added a Java section in https://codereview.chromium.org/2664133003/ as well to restore 100 chars for Java.

#9: probably, but we were unsure of which other languages could be affected and needed to be also added to the list (what about javascript? are there other languages using clang-format in Chromium? ...)

Given the uncertainties, we choose to roll back buildtools so that people more informed of how clang-format is used by Chromium can provide a better fix. From the discussion, it looks like a fix will need to land in clang-format. dcheng@ can you take care of this or assign the issue to a better owners?

Reducing the priority as it is no longer preventing iOS developer from working.

Rolling back was the right call. Should probably also be reverted in buildtools, else the next gn roll will regress this again.

> src/ios/ requires code to be formatted with clang-format

where is this enforced?  there is no ios/PRESUBMIT.py in my chromium checkout.  you did not mention this yesterday / when you filed the bug.

you basically left this bug in a completely unactionable state yesterday.  you assigned the bug to yourself.  you did not leave clear instructions on whether you wanted to move BACK to 80 columns (which seems suboptimal in the long term if it disagrees with a style guide) or move FORWARD to 100 cols.

additionally, as thakis@ mentioned, you reverted the roll but the next one will pull this back in.


> We tried suggestion in comment #2, but this impact java that is now formatted at 80 columns.

you tried it incorrectly; here's a CL for you:
https://codereview.chromium.org/2665183002/

with it applied:

$ clang-format -dump-config -assume-filename=some.mm | egrep 'ColumnLimit|Language'
Language:        ObjC
ColumnLimit:     80

$ clang-format --version
clang-format version 4.0.0 (trunk 290930)


> My suggestion was that we should fix presubmit check to respect what both the style guide and the auto formatter wants as line length limit for Objective-C source file and headers.

what's stopping us from changing the presubmit line length to 100 for .mm files?  it looks as easy as:
https://chromium-review.googlesource.com/#/c/434925/


> Regarding which extension to use for Objective-C header, we cannot use .m as this is used for Objective-C source file while .mm is used for Objective-C++ files (correspond to .c for C files and .cc for C++ files). Using anything besides .h will confuse our tooling (e.g. Xcode expects the header files to be named .h).

that's fine as long as you're OK with .h being treated as Language: Cpp (80 cols).

> where is this enforced?  there is no ios/PRESUBMIT.py in my chromium checkout.
> you did not mention this yesterday / when you filed the bug.

This is enforced only for ios/chrome and ios/web (in the corresponding PRESUBMIT.py). Sorry for not mentioning this initially. As I mostly contribute to src/ios/ it was obvious to me that the code had to be formatted by clang-format when uploaded for review.

> you basically left this bug in a completely unactionable state yesterday.
> you assigned the bug to yourself.  you did not leave clear instructions on
> whether you wanted to move BACK to 80 columns (which seems suboptimal in the
> long term if it disagrees with a style guide) or move FORWARD to 100 cols.

Sorry again for putting too much information in my bug. I though that the change to 100 columns introduced in clang-format was intended and had been discussed in a channel I was not following. As I though the change was intentional, I mentioned the style guide as I felt it may have been why the change was introduced to clang-format.

As I didn't knew whether the change was intentional I didn't knew what the proper fix was supposed to be. If a decision had been made to switch Objective-C to 100 columns then the correct fix would be to change the presubmit. If the change was not intentional, then the proper fix was to fix the configuration for clang-format (either in src/.clang-format or directly in the chromium style in clang-format).

I personally have no stake in what the column limit should be for Objective-C. I only think that the tool should not prevent developer from uploading their CL because they disagree on how the file should be formatted.
 
> additionally, as thakis@ mentioned, you reverted the roll but the next one
> will pull this back in.

buildtools does not have a CQ. My goal today was to unblock iOS developers. I knew that the change may roll forward again with some change. I was trying to buy some time so that we could think of what the proper fix was supposed to be.

> you tried it incorrectly; here's a CL for you:
> https://codereview.chromium.org/2665183002/
> 
> with it applied:
> 
> $ clang-format -dump-config -assume-filename=some.mm | egrep 'ColumnLimit|Language'
> Language:        ObjC
> ColumnLimit:     80
> 
> $ clang-format --version
> clang-format version 4.0.0 (trunk 290930)

Thank you for this CL. Maybe we can land it and then reland the roll.

> what's stopping us from changing the presubmit line length to 100 for
> .mm files?  it looks as easy as:
> https://chromium-review.googlesource.com/#/c/434925/

> that's fine as long as you're OK with .h being treated as Language:
> Cpp (80 cols).

Sadly, it's not as simple as increasing the limit for .mm files in presubmit.py. This was the first thing I tried but if foo.h and foo.mm were modified in the same CL, both would be reformatted to use 100 columns. The presubmit would still fail on foo.h then.

I'm the one who requested that something should be done to unlock iOS developers in EMEA time frame. Changing the line length to 100 chars was disruptive and impacting us significantly yesterday and today. We do rely on our presubmit, and on git cl format to reformat our code.

I'm not familiar with clang-format, based on your recommendation I made a CL to change the .clang-format at the top level however:

* The format I used was incorrect, I didn't realize that the --- were significant.
* it is impossible to get a timely code review in EMEA for anything at the top level, this would have been a risky TBR at best.

So I asked Sylvain to revert the roll because that was the only option left to us to get back on track in a timely manner. Note that I will press the revert button on the next roll if it causes ObjectiveC files to be formatted to 100 char.

From here there are two paths:

* We can land your CL to make the 80 char limit on ObjectiveC explicit on the top level .clang-format and then re-land the roll
* A fix should be done in clang-format to go back to 80 chars for objectiveC++ in the chromium spec, and then roll it into Chromium.

Keeping 100 chars is not an option at this time. We can have a discussion about it amongst ObjectiveC devs, but this will take time, and should not be acted upon unilaterally. 

To reiterate: We will not adopt 100 cols for chromium's objc. We discussed that years ago and decided against it. This was an unintentional change.

thakis@ was worried that changing src/.clang-format would leave other projects (i.e. skia) in the cold, so we did not go forward with that change:
https://codereview.chromium.org/2665183002/

I reverted the roll from buildtools/ so it's now unblocked / in a more consistent state:
https://codereview.chromium.org/2667853003/

I re-added 2 .clang-format files that depend on the new binaries (for the JS -style=Chromium defaults) with another revert:
https://codereview.chromium.org/2662323002/

Nico and I changed the upstream clang-format ObjC column limit back to 80 for Chromium:
http://llvm.org/viewvc/llvm-project?view=revision&revision=293675

Unfortunately, we're worried about rolling to my new rev because it'd start enforcing comment reflowing:
http://llvm.org/viewvc/llvm-project?view=revision&revision=293055

It's possible to turn it off in Format.cpp:
https://github.com/llvm-mirror/clang/blob/master/lib/Format/Format.cpp#L555

Or in a .clang-format file with:

  ReflowComments: false

but the same general issue occurs with src/.clang-format not applying everywhere (so other projects might be affected).

I think the notion of comment reflowing is generally a good idea, so disabling it in the binary itself might not be desired long term.


General note: sorry for the breakage; sorry for being frustrated earlier.  Building and uploading and rolling these binaries takes a long time (at least on your first time).  I also didn't know the scope of this issue, or I would've handled it more conservatively (reverted earlier).