andmis added a comment.
Thanks all for the reviews. I've updated the patch and added responses to your
comments.
================
Comment at: clang/docs/ClangFormatStyleOptions.rst:2820
+**JavaScriptWrapImports** (``Boolean``) :versionbadge:`clang-format 3.9`
+ Whether to wrap JavaScript import/export statements. If ``false``, then
----------------
MyDeveloperDay wrote:
> This file is generated from Format.h you need to make the changes there and
> regenerate using clang/docs/tools/dump_format_style.py
Alright. I've added the source to `Format.h` and run `dump_format_style.py`.
================
Comment at: clang/docs/ClangFormatStyleOptions.rst:2827
-**JavaScriptWrapImports** (``Boolean``) :versionbadge:`clang-format 3.9`
- Whether to wrap JavaScript import/export statements.
+ * If ``ColumnWidth`` is 0 (no limit on the number of columns), then import
+ statements will keep the number of lines they start with.
----------------
HazardyKnusperkeks wrote:
> mprobst wrote:
> > this seems odd to me: my understanding is that clang-format always reflows
> > the entire document, there's no logic to ever "keep" whitespace.
> >
> > Are you sure you are seeing this behaviour? The logic change below sounds
> > more as if the ColumnWidth: 0, import lines might not break (mustBreak
> > returns false), but might still break?
> From what I can tell with `ColumnLimit` (and not `ColumnWidth`) 0, then
> `clang-format` does many things differently. If by design or by accident I'm
> not sure. I can see that often, since I format my code with a limit, but my
> tests without.
I just double-checked and yes, as-implemented, with `ColumnLimit: 0` and
`JavaScriptWrapImports: true`, import statements retain the number of lines
they started with.
It is explicit in the code that with `ColumnLimit: 0`, we decide whether to
break on a token based on whether there was a preexisting line break. (Check
out `NoColumnLimitLineFormatter`.) I think you're right that usually
clang-format reflows everything and I was also confused about this point.
There are other options that do similar things, for example
`EmptyLineAfterAccessModifier`.
================
Comment at: clang/docs/ClangFormatStyleOptions.rst:2836
- true:
+ // Original
+ import {VeryLongImport, AnotherLongImport, LongImportsAreAnnoying} from
'some/module.js'
----------------
curdeius wrote:
> What does "Original" mean here? It's unclear whether the option is true or
> false nor what the ColumnWidth is.
>
> I think you want to have 3 examples:
> 1) JavaScriptWrapImports: false, independent of ColumnWidth
> 2) JavaScriptWrapImports: true, ColumnWidth: 0
> 3) JavaScriptWrapImports: true, ColumnWidth: non-zero
> In each case you want to have short and long import lines.
"Original" means "Before running clang-format", I've changed the comment to the
latter.
The reason this is here is that with `ColumnLimit: 0`, the output format
depends on the input, and I don't see how to make that clear without having a
before and after view.
================
Comment at: clang/unittests/Format/FormatTestJS.cpp:1948
+ Style.JavaScriptWrapImports = false;
verifyFormat("import {VeryLongImportsAreAnnoying,
VeryLongImportsAreAnnoying,"
" VeryLongImportsAreAnnoying, VeryLongImportsAreAnnoying"
----------------
MyDeveloperDay wrote:
> MyDeveloperDay wrote:
> > you are no longer testing the LLVM Case, please don't remove that, but feel
> > free to ensure they are doing the same for google!
> Its kind of our golden rule, don't change existing tests, subtle changes can
> have huge implications on large code bases
Thanks, this was an oversight on my part.
================
Comment at: clang/unittests/Format/FormatTestJS.cpp:1975
+ Style);
+ // Using the input_code/expected version of verifyFormat because we can't
+ // indiscriminately test::messUp on these tests.
----------------
mprobst wrote:
> I'm not sure this test case really repros the doc changes you made (keeps any
> line wraps with ColW: 0).
>
> I think if you wanted to demonstrate that, you'd need to add a test case
> where clang-format would normally make a change, and show that it does not
> with ColW: 0.
>
> However I think that's not feasible: by design, clang-format cannot not make
> a change, it always reformats all code. You might need to rethink the intent
> here.
I think the tests are doing what's claimed. I've rearranged and rewritten them,
PTAL.
================
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"
----------------
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.
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
