[PATCH] D28165: Change clang-format's Chromium JavaScript defaults

[Nico Weber via Phabricator via cfe-commits]

thakis closed this revision.
thakis added a comment.

http://llvm.org/viewvc/llvm-project?view=revision=290930


https://reviews.llvm.org/D28165



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28165: Change clang-format's Chromium JavaScript defaults

[Dan Beam via Phabricator via cfe-commits]

danbeam updated this revision to Diff 82989.
danbeam marked an inline comment as done.
danbeam added a comment.

setting up arc


https://reviews.llvm.org/D28165

Files:
  lib/Format/Format.cpp


Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -638,6 +638,9 @@
 ChromiumStyle.BreakAfterJavaFieldAnnotations = true;
 ChromiumStyle.ContinuationIndentWidth = 8;
 ChromiumStyle.IndentWidth = 4;
+  } else if (Language == FormatStyle::LK_JavaScript) {
+ChromiumStyle.AllowShortIfStatementsOnASingleLine = false;
+ChromiumStyle.AllowShortLoopsOnASingleLine = false;
   } else {
 ChromiumStyle.AllowAllParametersOfDeclarationOnNextLine = false;
 ChromiumStyle.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Inline;


Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -638,6 +638,9 @@
 ChromiumStyle.BreakAfterJavaFieldAnnotations = true;
 ChromiumStyle.ContinuationIndentWidth = 8;
 ChromiumStyle.IndentWidth = 4;
+  } else if (Language == FormatStyle::LK_JavaScript) {
+ChromiumStyle.AllowShortIfStatementsOnASingleLine = false;
+ChromiumStyle.AllowShortLoopsOnASingleLine = false;
   } else {
 ChromiumStyle.AllowAllParametersOfDeclarationOnNextLine = false;
 ChromiumStyle.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Inline;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28165: Change clang-format's Chromium JavaScript defaults

[Nico Weber via Phabricator via cfe-commits]

thakis accepted this revision.
thakis added a comment.
This revision is now accepted and ready to land.

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 :-)


https://reviews.llvm.org/D28165



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28165: Change clang-format's Chromium JavaScript defaults

[Dan Beam via Phabricator via cfe-commits]

danbeam marked an inline comment as done.
danbeam added inline comments.



Comment at: lib/Format/Format.cpp:643
+ChromiumStyle.AllowShortIfStatementsOnASingleLine = false;
+ChromiumStyle.AllowShortLoopsOnASingleLine = false;
+  }

thakis wrote:
> 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.)
> so that we just use google style for JS

we want to tweak Google style a little bit.  I mentioned this on the [[ 
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/OTEfsPvp0qc/UsaOTR9IEQAJ
 | 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.


https://reviews.llvm.org/D28165



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28165: Change clang-format's Chromium JavaScript defaults

[Dan Beam via Phabricator via cfe-commits]

danbeam updated this revision to Diff 82946.
danbeam added a comment.

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


https://reviews.llvm.org/D28165

Files:
  lib/Format/Format.cpp


Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -638,6 +638,9 @@
 ChromiumStyle.BreakAfterJavaFieldAnnotations = true;
 ChromiumStyle.ContinuationIndentWidth = 8;
 ChromiumStyle.IndentWidth = 4;
+  } else if (Language == FormatStyle::LK_JavaScript) {
+ChromiumStyle.AllowShortIfStatementsOnASingleLine = false;
+ChromiumStyle.AllowShortLoopsOnASingleLine = false;
   } else {
 ChromiumStyle.AllowAllParametersOfDeclarationOnNextLine = false;
 ChromiumStyle.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Inline;


Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -638,6 +638,9 @@
 ChromiumStyle.BreakAfterJavaFieldAnnotations = true;
 ChromiumStyle.ContinuationIndentWidth = 8;
 ChromiumStyle.IndentWidth = 4;
+  } else if (Language == FormatStyle::LK_JavaScript) {
+ChromiumStyle.AllowShortIfStatementsOnASingleLine = false;
+ChromiumStyle.AllowShortLoopsOnASingleLine = false;
   } else {
 ChromiumStyle.AllowAllParametersOfDeclarationOnNextLine = false;
 ChromiumStyle.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Inline;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28165: Change clang-format's Chromium JavaScript defaults

[Nico Weber via Phabricator via cfe-commits]

thakis added inline comments.



Comment at: lib/Format/Format.cpp:643
+ChromiumStyle.AllowShortIfStatementsOnASingleLine = false;
+ChromiumStyle.AllowShortLoopsOnASingleLine = false;
+  }

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.)


https://reviews.llvm.org/D28165



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28165: Change clang-format's Chromium JavaScript defaults

[Dan Beam via Phabricator via cfe-commits]

danbeam created this revision.
danbeam added a reviewer: thakis.
danbeam added a subscriber: cfe-commits.
Herald added a subscriber: klimek.

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++.


https://reviews.llvm.org/D28165

Files:
  lib/Format/Format.cpp


Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -639,13 +639,17 @@
 ChromiumStyle.ContinuationIndentWidth = 8;
 ChromiumStyle.IndentWidth = 4;
   } else {
+ChromiumStyle.AllowShortIfStatementsOnASingleLine = false;
+ChromiumStyle.AllowShortLoopsOnASingleLine = false;
+  }
+
+  if (Language == FormatStyle::LK_Cpp) {
 ChromiumStyle.AllowAllParametersOfDeclarationOnNextLine = false;
 ChromiumStyle.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Inline;
-ChromiumStyle.AllowShortIfStatementsOnASingleLine = false;
-ChromiumStyle.AllowShortLoopsOnASingleLine = false;
 ChromiumStyle.BinPackParameters = false;
 ChromiumStyle.DerivePointerAlignment = false;
   }
+
   ChromiumStyle.SortIncludes = false;
   return ChromiumStyle;
 }


Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -639,13 +639,17 @@
 ChromiumStyle.ContinuationIndentWidth = 8;
 ChromiumStyle.IndentWidth = 4;
   } else {
+ChromiumStyle.AllowShortIfStatementsOnASingleLine = false;
+ChromiumStyle.AllowShortLoopsOnASingleLine = false;
+  }
+
+  if (Language == FormatStyle::LK_Cpp) {
 ChromiumStyle.AllowAllParametersOfDeclarationOnNextLine = false;
 ChromiumStyle.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Inline;
-ChromiumStyle.AllowShortIfStatementsOnASingleLine = false;
-ChromiumStyle.AllowShortLoopsOnASingleLine = false;
 ChromiumStyle.BinPackParameters = false;
 ChromiumStyle.DerivePointerAlignment = false;
   }
+
   ChromiumStyle.SortIncludes = false;
   return ChromiumStyle;
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits