[PATCH] D17910: clang-format: [JS] Handle certain cases of ASI.

[Daniel Jasper via cfe-commits]

djasper added inline comments.

================
Comment at: lib/Format/UnwrappedLineParser.cpp:665
@@ +664,3 @@
+// well known cases. It *must not* return true in speculative cases.
+bool UnwrappedLineParser::shouldInsertSemiBetween(FormatToken *Previous,
+                                                  FormatToken *Next) {
----------------
Make the parameters const.

I'd also like this to have "JS" in the name somehow. And I am worried that this might be confusing because it says "shouldInsert", but we don't insert (at least not yet). Maybe rename this to wouldTriggerJavaScriptASI(...).

================
Comment at: lib/Format/UnwrappedLineParser.cpp:668
@@ +667,3 @@
+  bool IsOnSameLine =
+      CommentsBeforeNextToken.empty()
+          ? Next->NewlinesBefore == 0
----------------
I think it is a bit confusing to hand in both Previous and Next but still rely on some state of the class. Maybe it is better to move the 4 lines from nextToken into this function?

================
Comment at: lib/Format/UnwrappedLineParser.cpp:690
@@ +689,3 @@
+
+bool UnwrappedLineParser::isJavaScriptIdentifier(FormatToken *FormatTok) {
+  return FormatTok->is(tok::identifier) &&
----------------
I'd just hand in "Keywords" for now and make this a local (static) function. We should also refactor this whole thing to not have the class definition in the header probably.

Also, this is incorrect as it doesn't return true for C++ keywords that aren't JS keywords, e.g. try "struct". Leave a FIXME for me to sort this out.

================
Comment at: lib/Format/UnwrappedLineParser.cpp:1942
@@ -1898,2 +1941,3 @@
   pushToken(FormatTok);
+  FormatToken *Previous = FormatTok;
   readToken();
----------------
const?

================
Comment at: lib/Format/UnwrappedLineParser.cpp:1945
@@ +1944,3 @@
+  if (Style.Language == FormatStyle::LK_JavaScript &&
+      shouldInsertSemiBetween(Previous, FormatTok)) {
+    addUnwrappedLine();
----------------
In LLVM-style, we generally don't use braces for single-statement ifs.

================
Comment at: unittests/Format/FormatTestJS.cpp:56
@@ +55,3 @@
+
+  static void verifyFormatNoMessup(
+      llvm::StringRef Code,
----------------
I don't think it is useful to do this as there are always ways in which formatting can fail leaving the input untouched. We still want to verify that formatting takes place.

To do so, manually mess up the code in some way and use EXPECT_EQ. There should be various examples in FormatTest.cpp.


http://reviews.llvm.org/D17910