[PATCH] D17385: clang-format: [JS] re-quote single or double quoted strings.

[Daniel Jasper via cfe-commits]

djasper added inline comments.

================
Comment at: include/clang/Format/Format.h:606
@@ +605,3 @@
+  /// \brief The QuoteStyle to use for JavaScript strings.
+  QuoteStyle Quotes;
+
----------------
As this only affects JavaScript and doesn't make any sense for Java or C++, I'd like to have JS/JavaScript in the option and enum name.

================
Comment at: lib/Format/Format.cpp:76
@@ +75,3 @@
+  static void enumeration(IO &IO, FormatStyle::QuoteStyle &Value) {
+    IO.enumCase(Value, "leave", FormatStyle::QS_Leave);
+    IO.enumCase(Value, "single", FormatStyle::QS_Single);
----------------
For almost all the other enums we use upper case.

================
Comment at: lib/Format/Format.cpp:1087
@@ +1086,3 @@
+        (Style.Quotes == FormatStyle::QS_Single &&
+         !FormatTok->TokenText.startswith("\"")) ||
+        (Style.Quotes == FormatStyle::QS_Double &&
----------------
Are there more than the two options? Otherwise, I would find:

  Style.Quotes == FormatStyle::QS_Single && FormatTok->TokenText.startswith("\'")

more intuitive.

================
Comment at: lib/Format/Format.cpp:1094
@@ +1093,3 @@
+
+    StringRef Input = FormatTok->TokenText;
+    size_t ColumnWidth = FormatTok->TokenText.size();
----------------
Move this up so you can use it in the if-statement?

================
Comment at: unittests/Format/FormatTestJS.cpp:638
@@ -637,3 +637,3 @@
 TEST_F(FormatTestJS, StringLiteralConcatenation) {
-  verifyFormat("var literal = 'hello ' +\n"
-               "    'world';");
+  verifyFormat("var literal =\n"
+               "    'hello ' + 'world';",
----------------
Why change this test? I think this was meant to test that we always add a line break between two consecutive string literals as otherwise the user could just have written this without the "' + '".

================
Comment at: unittests/Format/FormatTestJS.cpp:878
@@ -876,3 +877,3 @@
                "  Y,\n"
-               "} from 'some/long/module.js';",
+               "} from\n    'some/long/module.js';",
                getGoogleJSStyleWithColumns(20));
----------------
I don't think this is intended.


http://reviews.llvm.org/D17385