This is an archive of the discontinued LLVM Phabricator instance.

Change clang-format's Chromium JavaScript defaults
ClosedPublic

Authored by danbeam on Dec 29 2016, 12:39 PM.

Details

Reviewers
thakis
Summary

Chromium is starting to use clang-format on more JavaScript.

In doing this, we discovered that our defaults were not doing a good job differentiating between JS and C++.

This change moves some defaults to only apply to C++.

Event Timeline

danbeam updated this revision to Diff 82689.Dec 29 2016, 12:39 PM
danbeam retitled this revision from to Change clang-format's Chromium JavaScript defaults.
danbeam updated this object.
danbeam added a reviewer: thakis.
danbeam added a subscriber: cfe-commits.
fhahn added a subscriber: fhahn.Dec 29 2016, 2:17 PM
thakis added inline comments.Jan 3 2017, 9:38 AM
lib/Format/Format.cpp
643

Thanks for the patch! Do we want these as false in Chromium's JS? I would've thought the diff would just be

- } else {
+ } else if (Language != FormatStyle::LK_JavaScript)

so that we just use google style for JS.

If we do want to deviate from google style here for some reason then
a) say why somewhere
b) change the check for cpp to also include || Language == FormatStyle::LK_ObjC

(If you include more diff context as described on http://llvm.org/docs/Phabricator.html, reviewing on phab is a bit easier.)

danbeam updated this revision to Diff 82946.Jan 3 2017, 1:54 PM

make a branch specific to JS and duplicate some format options for explicitness

danbeam marked an inline comment as done.Jan 3 2017, 1:59 PM
danbeam added inline comments.
lib/Format/Format.cpp
643

so that we just use google style for JS

we want to tweak Google style a little bit. I mentioned this on the chromium-dev@ thread.

I added a specific branch for JS with explicit specializations so it's clearer to the reader how Google and Chromium differ.

(If you include more diff context as described on http://llvm.org/docs/Phabricator.html, reviewing on phab is a bit easier.)

Done.

thakis accepted this revision.Jan 3 2017, 2:26 PM
thakis edited edge metadata.

I'd rather we don't deviate from google style at all, but this brings us closer from where we are to that, so lgtm :-)

This revision is now accepted and ready to land.Jan 3 2017, 2:26 PM
danbeam updated this revision to Diff 82989.Jan 3 2017, 6:27 PM
danbeam marked an inline comment as done.
danbeam edited edge metadata.

setting up arc