================
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/
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
