On Mon, Jun 30, 2014 at 4:24 PM, Alexander Kornienko <[email protected]> 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 > > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
