[PATCH] D60605: [clangd] Revamp textDocument/onTypeFormatting.

Mon Jun 10 07:21:42 PDT 2019

sammccall marked 3 inline comments as done.
sammccall added a comment.

OK, this has been stuck for a while and (as discussed a bit offline) I haven't been able to make the alternative approaches work.

In D60605#1495268 <https://reviews.llvm.org/D60605#1495268>, @yvvan wrote:

> @ilya-biryukov 
>  What do you think about D53072 <https://reviews.llvm.org/D53072>? It can be polished and combined with this change removing some code from here (which I assume is a good thing).
>  The idea there is that clang-format knows that it's not allowed to remove new lines and it always marks them with MustBreakBefore.
>  I'm not sure if anybody needs an executable but I think it's not a big deal to have an extra reformat() function.
>
> It is also safe because it was the only way I found which does not break the code style like, for instance, adding comment block to the end of the previous line.

I'm not sure D53072 <https://reviews.llvm.org/D53072> helps in its current form. Preserving newlines with content on the line isn't always the (IMO) right formatting: see e.g. FormatBrace test.
(I have other concerns about the details of that patch, but they don't really relate to this one).

In D60605#1518031 <https://reviews.llvm.org/D60605#1518031>, @ilya-biryukov wrote:

> LGTM with a few NITs and a few questions about the possibility of moving this to `clang-format` at some point in the future.
>  From what I can to simplify the calling, we need to:
>
> 1. teach `clang-format` about a cursor (by making it a special token or somehow else), this would allow to avoid some book-keeping;

There's at least one wrinkle with making it a special token: it may be within a token. E.g. after splitting a line comment, the cursor is at offset 3 within `// the new line`. Not sure how deep this problem goes.

> 2. teach `clang-format` to format in presence of unmatched parentheses. Again, would simplify things. Although not sure it's easy to do in `clang-format` either;

Note this needs to be a style or other option, as the fact that clang-format bails out on broken code in general is well-established desired behavior.
I attempted this (by having the parser return "true" rather than "false" on reaching the end of a paren/brace list, and got lost in the details. I'm not sure whether it's likely to work, or whether clang-format would just do the same source transformation as we do here. (Obviously sharing it would be nice).

> 3. teach `clang-format` to respect indentation before the cursor (added by the editor in our case).

I'm not sure what you mean by this: I don't think we want that indentation to be respected, but rather reformatted away.

> After that we don't seem to need the complicated multi-pass replacements.

We currently have pre-formatting changes (e.g. comment splitting). I'm not sure if you expect these to belong to clang-format, but I wouldn't. (They're more like a "respond to newline" helper for editors than they are like formatting a range). So you'd go from {pre-formatting changes, insert placeholder, clang-format, remove placeholder} to {pre-formatting changes, clang-format}.
So the logic to implement multi-pass replacements is still needed I think, though you'd have fewer actual passes.

> - Am I missing something that we also need from `clang-format` here?

The comment splitting and {} splitting, I think.

> - How would you estimate the amount of work needed to move this functionality to `clang-format`?
> - In general, do you think it's worth moving it there or not?

I don't know, I put maybe 8 hours into this approach and didn't make any progress.
If someone finds a clean way to do it, I'd be in favour. But a hacky/brute-force way would do more harm than good in my opinion.

> In the meantime, I've found another case where newline gets eaten:

Yes :-(
I've added a test to document this bad behavior. It's very hard to fix with clang-format as a black-box, because it hard-codes erasure of blank lines adjacent to a formatted region.
Probably the simplest approach is to add a clang-format option to always preserve blank lines. Although the motivation isn't an actual style guide, it makes sense to think of this as a style rule, it's very simple, and it's related to existing options.

================
Comment at: clangd/Format.h:25
+/// Applies limited formatting around new \p InsertedText.
+/// The \p Code already contains the updated text near \p Cursor, and may have
+/// had additional / characters (such as indentation) inserted by the editor.
----------------
ilya-biryukov wrote:
> NIT: maybe be more concrete? `near \p Cursor` means `right before \p Cursor`? Or can it have other characters (e.g. whitespace in your example)?
s/near/before/.
Yes, it can have other characters, these are already mentioned in the comment.

Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60605/new/

https://reviews.llvm.org/D60605