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

[Manuel Klimek]

On Wed, May 6, 2015 at 7:20 PM Daniel Jasper <djasper at 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


> 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.

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.


>
> ================
> 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/
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150506/e0e0b4d1/attachment.html>