curdeius added inline comments.
================ Comment at: clang/include/clang/Format/Format.h:2620 + /// + /// * If ``ColumnWidth`` is 0 (no limit on the number of columns), + /// then import statements will keep the number of lines they ---------------- Please change all occurrences of `ColumnWidth` (old name) to `ColumnLimit`. ================ Comment at: clang/unittests/Format/FormatTestJS.cpp:1990 Style.ColumnLimit = 40; - // Using this version of verifyFormat because test::messUp hides the issue. + Style.JavaScriptWrapImports = true; verifyFormat("import {\n" ---------------- andmis wrote: > mprobst wrote: > > curdeius wrote: > > > It's already true, cf. line 1977. Remove this line. > > FWIW, I think the tests might be more readable if each configuration > > ({true, false} x {col: 0, col: 40}) explicitly set all the options, even if > > it's a bit redundant. > I've reformatted them in two big blocks: one where `ColumnLimit > 0` and one > where `ColumnLimit: 0`. So there are no more redundant option set statements. > > This is the most-readable way to format the tests IMO because when > `ColumnLimit > 0`, we can use the simple `verifyFormat` (where you just pass > the expected result and it messes it up, formats, and checks) because > clang-format will do a full reflow. But for the `ColumnLimit: 0` tests, we > need to use the other `verifyFormat` (where we have input and expected output > explicitly), and it's convenient to put a comment explaining what's happening > at the top of the `ColumnLimit: 0` block. It's ok for me too, but I'd like to either put all the involved options each time, or minimalistic changes, but not a mix of the two. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116638/new/ https://reviews.llvm.org/D116638 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits