On Wed, May 6, 2015 at 7:36 PM, Manuel Klimek <kli...@google.com> wrote:
>
>
> On Wed, May 6, 2015 at 7:20 PM Daniel Jasper <djas...@google.com> wrote:
>
>> ================
>> 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.
>>
>
> I think it's a useful separation in that:
> a) I'd want to eventually pull out a class
> 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
> c) I don't think lambdas would help much
>
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.
b) A lambda would achieve the same thing.
c) see b).
> 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.
>
>
> 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.
>
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.
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.
>
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.
>
>> ================
>> 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.
>>
>
> This came from trying to understand what it was actually doing (format is
> very broad in this case).
>
>
>> ================
>> 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
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
