[PATCH] clang-format: Add reflow capability to line comment formatting

[Manuel Klimek]

On Mon, Jun 30, 2014 at 4:24 PM, Alexander Kornienko <alexfh at google.com>
wrote:

> Thanks for working on this!
>
> Before I do a thorough review, I'd like you to address a few high-level
> issues and remove commented code (it shouldn't make it to the repository
> anyway, and it doesn't help understanding the intentions, FIXME comments
> work better).
>
> It would be more convenient for reviewers if you generated diff using
> "diff -u10000" to include all context. Alternatively, you could use the
> Phabricator's arcanist command-line tool to send your patch for review.
> Both are fine, choose whatever works better for you.
>
> Second, I would ask you to add your tests to
> unittest/Format/FormatTest.cpp and run the whole test suite. Unfortunately,
> I have no idea on how to run clang unittests in Windows. In Linux depending
> on the way the project is built, running all Clang tests can be done in one
> of the following ways:
>   * in CMake+make builds - "make check-clang"
>   * in configure+make builds it should also be "make check-clang"
>   * in CMake+ninja builds it can be done using "ninja check-all", iirc.
>
> I'm not sure if the steps described in
> http://llvm.org/docs/GettingStartedVS.html are enough to run unit tests
> or not. The best way to know is to write a failing test and to try. If you
> don't find a way to run the unit tests on Windows, you can always install a
> copy of Ubuntu on a virtual machine and use it for testing.
>

It's quite easy to run unit tests on windows: you just execute one of the
unit test binaries (in the case of the formatter the FormatTests)


>
> Could you also test your patch on some real code? LLVM/Clang sources can
> be a good test case, just reduce the column limit (to 60 or 30, for
> example) to have the line breaking/merging code work. This way you can find
> many test cases you didn't think about.
>
> For example, we need to avoid joining lines with code and various
> ascii-art, even when the column limit is exceeded and they end up being
> broken. I'd go with even more conservative approach: detect boundaries of
> what looks like paragraphs of text, and join lines only within those
> boundaries. I'm not sure if this fits into your current approach, and this
> is why I suggested to parse consecutive block comments as a single
> BreakableToken. This way one could also implement reflowing of block
> comments in a similar manner.
>
> See a few more random comments inline.
>
> ================
> Comment at: lib/Format/ContinuationIndenter.cpp:947
> @@ -937,1 +946,3 @@
>                                                      bool DryRun) {
> +   const bool IsReflowingLineComments = [&] {
> +    FormatToken *Previous = Current.Previous;
> ----------------
> Any reason to do use lambda here?
>
> ================
> Comment at: lib/Format/ContinuationIndenter.cpp:1066
> @@ -1022,1 +1065,3 @@
> +    }
> +    if (!DryRun) {
>        Token->replaceWhitespaceBefore(LineIndex, Whitespaces);
> ----------------
> We don't use braces around single-line "if" bodies.
>
> ================
> Comment at: lib/Format/ContinuationIndenter.cpp:1111
> @@ -1054,3 +1110,3 @@
>        assert(NewRemainingTokenColumns < RemainingTokenColumns);
> -      if (!DryRun)
> +      if (!DryRun) {
>          Token->insertBreak(LineIndex, TailOffset, Split, Whitespaces);
> ----------------
> We don't use braces around single-line "if" bodies.
>
> ================
> Comment at: lib/Format/ContinuationIndenter.cpp:1121
> @@ -1065,2 +1120,3 @@
>        BreakInserted = true;
> +
>      }
> ----------------
> Please remove the empty line.
>
> ================
> Comment at: lib/Format/Format.cpp:586
> @@ +585,3 @@
> +    if (TheLine->Last->Type == TT_LineComment) {
> +      auto HasOnlyWhitespaceTokens = [](StringRef Text) {
> +        StringRef RTrimmedCommentText = Text.rtrim();
> ----------------
> This name doesn't say what the function really does. You probably meant
> "character" not "token". Token is a lexical entity, e.g. the whole comment.
> And the implementation is suboptimal. If you want to check that the comment
> has a form of any number of slashes followed by any number of spaces, then
> you could write:
>
>   Text.rtrim().ltrim("/").empty();
>
> instead.
>
> What are the cases you want this function to detect?
>
> ================
> Comment at: lib/Format/Format.cpp:1178
> @@ -1105,2 +1177,3 @@
>          continue;
> -
> +      const bool IsReflowingLineComments = [&] {
> +        const FormatToken *CurrentTok = Node->State.NextToken;
> ----------------
> Any reason to do use lambda here?
>
> ================
> Comment at: tools/clang-format/ClangFormat.cpp:307
> @@ +306,3 @@
> +  // there must be a way to have all editors identify this?
> +  clang::format::IsInEditorMode =
> +      Offsets.getNumOccurrences() && Lengths.getNumOccurrences();
> ----------------
> I'd rather add a separate style option controlling how/when we want to
> join comments (say, ReflowComments=Never|LongLines|Always). This way what
> "editor mode" means could be configured externally and explicitly.
>
> ================
> Comment at: tools/clang-format/ClangFormat.cpp:330
> @@ -320,1 +329,2 @@
>  }
> +
> ----------------
> Please remove trailing empty lines here and in other places.
>
> http://reviews.llvm.org/D4345
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140630/c13b7866/attachment.html>