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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
