<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, May 6, 2015 at 7:36 PM, Manuel Klimek <span dir="ltr"><<a href="mailto:klimek@google.com" target="_blank" class="cremed">klimek@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><br><div class="gmail_quote"><div><div class="h5">On Wed, May 6, 2015 at 7:20 PM Daniel Jasper <<a href="mailto:djasper@google.com" target="_blank" class="cremed">djasper@google.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">================<br>
Comment at: include/clang/Format/Format.h:550<br>
@@ +549,3 @@<br>
+///<br>
+/// If \c SkippedLines is non-null, its value will be set to true if any<br>
+/// of the affected lines were not formatted due to a non-recoverable syntax<br>
----------------<br>
I would not call this "skipped lines" as that might lead to confusion. Especially, if you format:<br>
<br>
"int a; f (;"<br>
<br>
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.<br>
<br>
I think the one bit we are trying to get out here is something like "FoundUnrecoverableError".<br>
<br>
<br>
<br>
<br>
================<br>
Comment at: lib/Format/UnwrappedLineFormatter.cpp:470<br>
@@ +469,3 @@<br>
+<br>
+ } else if (ShouldFormat && SkippedLines) {<br>
+ *SkippedLines = true;<br>
----------------<br>
Don't use else after return.<br>
<br>
================<br>
Comment at: lib/Format/UnwrappedLineFormatter.h:76<br>
@@ +75,3 @@<br>
+ /// \c Indent and \c IndentForLevel will be updated.<br>
+ unsigned formatLine(const AnnotatedLine &TheLine,<br>
+ const AnnotatedLine *PreviousLine,<br>
----------------<br>
I think this is a bit of an anti-pattern:<br>
* You are passing in a lot of values, most of them 1:1 from local variables.<br>
* We can't come up with a better name than that of the only function calling this.<br>
* This function can only ever be usefully called from the other formatLine function.<br>
<br>
All of these hint at this not being a useful separation.<br></blockquote><div><br></div></div></div><div>I think it's a useful separation in that:</div><div>a) I'd want to eventually pull out a class</div><div>b) the point is that we see what is actually changed inside the loop and make the complexity of the problem explicit instead of hiding it inside one huge function; it took me way too long to figure out what goes on, so for me personally this does already help a lot and is a step in the right direction</div><div>c) I don't think lambdas would help much</div></div></div></blockquote><div><br></div><div>a) I agree that pulling out specific things into classes might help here, but I don't see how this is an intermediate step towards that.</div><div>b) A lambda would achieve the same thing.</div><div>c) see b).</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><span class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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.</blockquote><div><br></div></span><div>For me it's worth it, because figuring out what happens in the huge function is a large pain; figuring out what happens in smaller functions is significantly simpler to me, especially as the arguments make it explicit what's going on.</div></div></div></blockquote><div><br></div><div>But separating this into two functions doesn't really help as you are now passing 5+ arguments by pointer/reference. Still basically all variables can be modified pretty much anywhere, except now I have to look at two function. This separation to me is nothing more than basically drawing a horizontal line in the code and saying we only use these N variables from here on out. Except that you are polluting the interface with a function that I wouldn't have the first clue as to how to use them even with the comment.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div>Now, I agree that this is not the perfect solution, and that we probably could find a better rearchitecturing of the whole flow here, but I don't think that means we should leave way-too-long functions around.</div></div></div></blockquote><div><br></div><div>To me, this separation makes the situation worse and I'd prefer to leave this as is. Happy to take a look myself at how we can make improve the situation.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><span class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
================<br>
Comment at: lib/Format/UnwrappedLineFormatter.h:85<br>
@@ -70,3 +84,3 @@<br>
/// If \p DryRun is \c false, directly applies the changes.<br>
- unsigned format(const AnnotatedLine &Line, unsigned FirstIndent,<br>
- bool DryRun);<br>
+ unsigned formatLine(const AnnotatedLine &Line, unsigned FirstIndent,<br>
+ bool DryRun);<br>
----------------<br>
I don't see the point in renaming this, but ok.<br></blockquote><div><br></div></span><div>This came from trying to understand what it was actually doing (format is very broad in this case).</div><span class=""><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
================<br>
Comment at: unittests/Format/FormatTest.cpp:61<br>
@@ +60,3 @@<br>
+ const FormatStyle &Style = getLLVMStyle(),<br>
+ bool ExpectSkippedLines = false) {<br>
+ EXPECT_EQ(Code.str(),<br>
----------------<br>
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?<br>
<br>
================<br>
Comment at: unittests/Format/FormatTest.cpp:78<br>
@@ +77,3 @@<br>
+ const FormatStyle &Style = getLLVMStyle(),<br>
+ bool ExpectSkippedLines = false) {<br>
+ format(Code, Style, ExpectSkippedLines);<br>
----------------<br>
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.<br>
<br>
<a href="http://reviews.llvm.org/D9532" target="_blank" class="cremed">http://reviews.llvm.org/D9532</a><br>
<br>
EMAIL PREFERENCES<br>
<a href="http://reviews.llvm.org/settings/panel/emailpreferences/" target="_blank" class="cremed">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br>
<br>
<br>
</blockquote></span></div></div>
</blockquote></div><br></div></div>