[PATCH] Implements a way to retrieve information about whether some lines werenot formatted due to syntax errors.

[Daniel Jasper]

================
Comment at: include/clang/Format/Format.h:550
@@ +549,3 @@
+///
+/// If \c SkippedLines is non-null, its value will be set to true if any
+/// of the affected lines were not formatted due to a non-recoverable syntax
----------------
I would not call this "skipped lines" as that might lead to confusion. Especially, if you format:

  "int a; f (;"

the "line" will not be skipped entirely. And at some stage we actually might decide that we can't fix everything in an unwrapped line, but might still be able to do something.

I think the one bit we are trying to get out here is something like "FoundUnrecoverableError".




================
Comment at: lib/Format/UnwrappedLineFormatter.cpp:470
@@ +469,3 @@
+
+  } else if (ShouldFormat && SkippedLines) {
+    *SkippedLines = true;
----------------
Don't use else after return.

================
Comment at: lib/Format/UnwrappedLineFormatter.h:76
@@ +75,3 @@
+  /// \c Indent and \c IndentForLevel will be updated.
+  unsigned formatLine(const AnnotatedLine &TheLine,
+                      const AnnotatedLine *PreviousLine,
----------------
I think this is a bit of an anti-pattern:
* You are passing in a lot of values, most of them 1:1 from local variables.
* We can't come up with a better name than that of the only function calling this.
* This function can only ever be usefully called from the other formatLine function.

All of these hint at this not being a useful separation.

If you want to decrease indentation and want to use early exits, I think a local lambda might be the better choice here. If you are solely trying to make this function shorter, I don't think it is worth it.

================
Comment at: lib/Format/UnwrappedLineFormatter.h:85
@@ -70,3 +84,3 @@
   /// If \p DryRun is \c false, directly applies the changes.
-  unsigned format(const AnnotatedLine &Line, unsigned FirstIndent,
-                  bool DryRun);
+  unsigned formatLine(const AnnotatedLine &Line, unsigned FirstIndent,
+                      bool DryRun);
----------------
I don't see the point in renaming this, but ok.

================
Comment at: unittests/Format/FormatTest.cpp:61
@@ +60,3 @@
+                    const FormatStyle &Style = getLLVMStyle(),
+                    bool ExpectSkippedLines = false) {
+    EXPECT_EQ(Code.str(),
----------------
I don't think it makes sense to allow for combinations of style and ExpectSkippedLines. Skipped lines should be independent of the style. Lets instead introduce a separate function. Maybe veryFormatWithError or something?

================
Comment at: unittests/Format/FormatTest.cpp:78
@@ +77,3 @@
+                     const FormatStyle &Style = getLLVMStyle(),
+                     bool ExpectSkippedLines = false) {
+    format(Code, Style, ExpectSkippedLines);
----------------
I don't think it makes sense to test this. No crash tests are rare and whether we also discover an unrecoverable syntax error or not isn't really of interest.

http://reviews.llvm.org/D9532

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/