<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Jun 30, 2014 at 4:24 PM, Alexander Kornienko <span dir="ltr"><<a href="mailto:alexfh@google.com" target="_blank">alexfh@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Thanks for working on this!<br>
<br>
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).<br>
<br>
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.<br>
<br>
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:<br>
* in CMake+make builds - "make check-clang"<br>
* in configure+make builds it should also be "make check-clang"<br>
* in CMake+ninja builds it can be done using "ninja check-all", iirc.<br>
<br>
I'm not sure if the steps described in <a href="http://llvm.org/docs/GettingStartedVS.html" target="_blank">http://llvm.org/docs/GettingStartedVS.html</a> 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.<br>
</blockquote><div><br></div><div>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)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
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.<br>
<br>
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.<br>
<br>
See a few more random comments inline.<br>
<br>
================<br>
Comment at: lib/Format/ContinuationIndenter.cpp:947<br>
@@ -937,1 +946,3 @@<br>
bool DryRun) {<br>
+ const bool IsReflowingLineComments = [&] {<br>
+ FormatToken *Previous = Current.Previous;<br>
----------------<br>
Any reason to do use lambda here?<br>
<br>
================<br>
Comment at: lib/Format/ContinuationIndenter.cpp:1066<br>
@@ -1022,1 +1065,3 @@<br>
+ }<br>
+ if (!DryRun) {<br>
Token->replaceWhitespaceBefore(LineIndex, Whitespaces);<br>
----------------<br>
We don't use braces around single-line "if" bodies.<br>
<br>
================<br>
Comment at: lib/Format/ContinuationIndenter.cpp:1111<br>
@@ -1054,3 +1110,3 @@<br>
assert(NewRemainingTokenColumns < RemainingTokenColumns);<br>
- if (!DryRun)<br>
+ if (!DryRun) {<br>
Token->insertBreak(LineIndex, TailOffset, Split, Whitespaces);<br>
----------------<br>
We don't use braces around single-line "if" bodies.<br>
<br>
================<br>
Comment at: lib/Format/ContinuationIndenter.cpp:1121<br>
@@ -1065,2 +1120,3 @@<br>
BreakInserted = true;<br>
+<br>
}<br>
----------------<br>
Please remove the empty line.<br>
<br>
================<br>
Comment at: lib/Format/Format.cpp:586<br>
@@ +585,3 @@<br>
+ if (TheLine->Last->Type == TT_LineComment) {<br>
+ auto HasOnlyWhitespaceTokens = [](StringRef Text) {<br>
+ StringRef RTrimmedCommentText = Text.rtrim();<br>
----------------<br>
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:<br>
<br>
Text.rtrim().ltrim("/").empty();<br>
<br>
instead.<br>
<br>
What are the cases you want this function to detect?<br>
<br>
================<br>
Comment at: lib/Format/Format.cpp:1178<br>
@@ -1105,2 +1177,3 @@<br>
continue;<br>
-<br>
+ const bool IsReflowingLineComments = [&] {<br>
+ const FormatToken *CurrentTok = Node->State.NextToken;<br>
----------------<br>
Any reason to do use lambda here?<br>
<br>
================<br>
Comment at: tools/clang-format/ClangFormat.cpp:307<br>
@@ +306,3 @@<br>
+ // there must be a way to have all editors identify this?<br>
+ clang::format::IsInEditorMode =<br>
+ Offsets.getNumOccurrences() && Lengths.getNumOccurrences();<br>
----------------<br>
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.<br>
<br>
================<br>
Comment at: tools/clang-format/ClangFormat.cpp:330<br>
@@ -320,1 +329,2 @@<br>
}<br>
+<br>
----------------<br>
Please remove trailing empty lines here and in other places.<br>
<br>
<a href="http://reviews.llvm.org/D4345" target="_blank">http://reviews.llvm.org/D4345</a><br>
<br>
<br>
</blockquote></div><br></div></div>