[clang-format] Replace IncompleteFormat by a struct with Line
ClosedPublic

Authored by krasimir on Thu, Apr 20, 9:35 AM.

Details

Summary

This patch replaces the boolean IncompleteFormat that is used to notify the client if an unrecoverable syntax error occurred by a struct that also contains a line number.

Diff Detail

Repository
rL LLVM
krasimir created this revision.Thu, Apr 20, 9:35 AM
krasimir updated this revision to Diff 95974.Thu, Apr 20, 9:39 AM
  • Updated string escaping
krasimir updated this revision to Diff 95975.Thu, Apr 20, 9:43 AM
  • Removed comments
krasimir added a subscriber: cfe-commits.
djasper added inline comments.Thu, Apr 20, 11:12 PM
include/clang/Format/Format.h
1527 ↗(On Diff #95975)

This is a public interface function that possibly we shouldn't just change in case people use clang-format as a library.

Can you leave the old one, with a comment say that it is being deprecated. That one can then just call this new version and set the boolean depending on whether the string is empty.

lib/Format/UnwrappedLineFormatter.cpp
845 ↗(On Diff #95975)

I wonder whether this might be confusing when an unwrapped line spans over multiple physical lines. But I also don't have a much better idea on how to phrase this.

tools/clang-format/ClangFormat.cpp
314 ↗(On Diff #95975)

I think we should not change the JSON here in such a backwards incompatible way. That will break all existing editor integrations that use it. How about adding an additional field "Message:" that gets set when the format is incomplete?

krasimir updated this revision to Diff 96113.Fri, Apr 21, 2:43 AM
  • Address review comments
krasimir updated this revision to Diff 96115.Fri, Apr 21, 2:47 AM
krasimir marked an inline comment as done.
  • Removed double declaration
lib/Format/UnwrappedLineFormatter.cpp
845 ↗(On Diff #95975)

We may wait and see how much people will be confused.

krasimir marked an inline comment as done.Fri, Apr 21, 2:47 AM
krasimir updated this revision to Diff 96120.Fri, Apr 21, 3:58 AM
  • Introduce a proper abstraction
krasimir retitled this revision from [clang-format] Turn IncompleteFormat into a string to [clang-format] Replace IncompleteFormat by a struct with Line.Fri, Apr 21, 4:01 AM
krasimir edited the summary of this revision. (Show Details)
djasper added inline comments.Fri, Apr 21, 4:51 AM
include/clang/Format/Format.h
1519 ↗(On Diff #96120)

Maybe we should invert this and make this FormatComplete or something? Otherwise this is bound to lead to an unnecessary double-negative.

1522 ↗(On Diff #96120)

s/when/at which/

1523 ↗(On Diff #96120)

no comma, I think.

1524 ↗(On Diff #96120)

Hm. Something we might need to be thinking about here is whether this should be the line before or after formatting. So specifically, if I format:

int a
    = 1;
f(

Then, this gets reformatted to:

int a = 1;
f(

Would we want the Line to be 2 or 3?

lib/Format/Format.cpp
1886 ↗(On Diff #96120)

nit: There shouldn't be a line break here.

1889 ↗(On Diff #96120)

In LLVM style, there should be a line break here.

krasimir updated this revision to Diff 96129.Fri, Apr 21, 6:27 AM
krasimir marked 5 inline comments as done.
  • Changed IncompleteFormat to FormatComplete
krasimir updated this revision to Diff 96131.Fri, Apr 21, 6:37 AM
  • Refactor tests
krasimir added inline comments.Fri, Apr 21, 6:38 AM
include/clang/Format/Format.h
1524 ↗(On Diff #96120)

I'd go with the original line, because I think it might be more useful and because it's easier to implement.

krasimir added inline comments.Fri, Apr 21, 7:30 AM
include/clang/Format/Format.h
1524 ↗(On Diff #96120)

Even better: the line after formatting is functionally dependent on the line before formatting and the replacements. I see two use-cases: clients that refuse to format in case of syntax error, then they can just use the original line, and clients that apply the partial replacements, which can compute the line after formatting from them and the original line.

djasper accepted this revision.Fri, Apr 21, 7:34 AM

Sounds good.

This revision is now accepted and ready to land.Fri, Apr 21, 7:34 AM
This revision was automatically updated to reflect the committed changes.