This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Format tweak's replacements.
ClosedPublic

Authored by hokein on Feb 5 2019, 2:27 AM.

Details

Reviewers
ilya-biryukov

Event Timeline

hokein created this revision.Feb 5 2019, 2:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 5 2019, 2:27 AM
hokein marked an inline comment as done.Feb 5 2019, 2:32 AM
hokein added inline comments.
clangd/ClangdServer.cpp
366

not sure the err-handling strategy here -- maybe if this is failed, we still apply replacements (without formatting), rather than stopping.

ioeric added inline comments.Feb 5 2019, 2:37 AM
clangd/ClangdServer.cpp
366

You should use getFormatStyleForFile from SourceCode.h

Could we move the code to the Tweak itself, so that all the callers would get the formatting? I.e.

class Tweak {
   Replacements apply() {  // This is now non-virtual.
     auto Replacements = doApply();
     return cleanupAndFormat(Replacements);
   }

protected:
   virtual Replacements doApply() = 0; // inheritors should implement this now. Note: the name is terrible, suggestions are welcome.
};

This would ensure the callers don't need to deal with the formatting on their own.

clangd/ClangdServer.cpp
362

We can get the contents from InpAST.Inputs.Contents, no need to add an extra parameter.
Moving getFormatStyle() into the callback body should be ok.

500

Could this function get a vfs::FileSystem as a parameter?
Would server as a hint to the readers that it accesses the filesystem and allow to reuse vfs instances when they're already available (the apply tweaks case)

clangd/ClangdServer.h
267

NIT: could we swap the parameters for consistency with the other functions in this class (i.e. File goes first)

clangd/Format.h
29

NIT: add newline at the end of file.

unittests/clangd/TweakTests.cpp
82

NIT: remove the parameter, this should be the default.
At least until we run into the checks that deliberately don't want the formatting

110

NIT: maybe prefer LLVM style? To avoid context-switching.

ilya-biryukov added inline comments.Feb 5 2019, 3:05 AM
clangd/ClangdServer.cpp
366

not sure the err-handling strategy here -- maybe if this is failed, we still apply replacements (without formatting), rather than stopping.

Returning an error seems fine, this probably shouldn't happen under normal conditions and failing early means we're more likely to find the root-cause of the problem.

hokein updated this revision to Diff 185306.Feb 5 2019, 7:18 AM
hokein marked 9 inline comments as done.

Adress review comments.

hokein added a comment.Feb 5 2019, 7:19 AM

Could we move the code to the Tweak itself, so that all the callers would get the formatting? I.e.

class Tweak {
   Replacements apply() {  // This is now non-virtual.
     auto Replacements = doApply();
     return cleanupAndFormat(Replacements);
   }

protected:
   virtual Replacements doApply() = 0; // inheritors should implement this now. Note: the name is terrible, suggestions are welcome.
};

This would ensure the callers don't need to deal with the formatting on their own.

This seems introduce intrusive changes to the Tweak interface (Tweak will need to know the FormatStyle somehow), I think this might be done in another patch.

clangd/ClangdServer.cpp
366

You should use getFormatStyleForFile from SourceCode.h

Thanks for pointing it out! Didn't notice this method. I think we need to cache it somehow rather than calling this method every time.

Returning an error seems fine, this probably shouldn't happen under normal conditions and failing early means we're more likely to find the root-cause of the problem.

I switched to use getFormatStyleForFile, it seems a reasonable API.

clangd/ClangdServer.h
267

removed, not needed any more.

unittests/clangd/TweakTests.cpp
82

I'm ok to make this change. I tempt to keep it because it would make writing test code easier -- if we always format the code, we might spend time on figuring out the format diff between Before and After, which is not worth.

110

good catch.

This seems introduce intrusive changes to the Tweak interface (Tweak will need to know the FormatStyle somehow), I think this might be done in another patch.

It's totally fine, since we have just a single tweak now and changing the interface costs nothing.
The important part is making it hard to misuse the interface and forcing formatting on all the users of the interface is a significant burden. E.g. other users of ClangdServer (tests, users of clangd C++ API) shouldn't worry about formatting on their own.

hokein updated this revision to Diff 185529.Feb 6 2019, 4:54 AM

Move format to the tweak.

hokein marked an inline comment as done.Feb 6 2019, 4:56 AM
hokein added inline comments.
clangd/refactor/Tweak.h
81 ↗(On Diff #185529)

I think the current name is slightly better than doApply, I also have a few candidates in mind perform, invoke.

ilya-biryukov added inline comments.Feb 6 2019, 5:08 AM
clangd/refactor/Tweak.h
56 ↗(On Diff #185529)

NIT: Maybe make this a second argument of apply?
This would convey the idea that execute() should not do formatting on its own.

72 ↗(On Diff #185529)

Could you duplicate the EXPECTS: comments here? It's an important part of the public API.

ilya-biryukov added inline comments.Feb 6 2019, 5:24 AM
clangd/refactor/Tweak.h
81 ↗(On Diff #185529)

execute() LGTM!

hokein updated this revision to Diff 185530.Feb 6 2019, 5:36 AM
hokein marked 3 inline comments as done.

Address review comments.

hokein updated this revision to Diff 185531.Feb 6 2019, 5:36 AM

Add missing file.

hokein added inline comments.Feb 6 2019, 5:37 AM
clangd/refactor/Tweak.h
56 ↗(On Diff #185529)

SG, this also makes the format intention of apply more explicitly.

ilya-biryukov accepted this revision.Feb 6 2019, 6:35 AM

LGTM! Also a few NITs.

clangd/ClangdServer.cpp
154

NIT: put . to the end of the comment.

378–379

This FIXME is stale now, remove?

clangd/Format.h
20

NIT: Maybe move to SourceCode.h? We have helpers to turn replacements into edits there, having this would be a good addition.
Or inline now that this function has only one call-site?

The extra file does not seem to carry its weight.

This revision is now accepted and ready to land.Feb 6 2019, 6:35 AM
hokein updated this revision to Diff 185550.Feb 6 2019, 7:18 AM
hokein marked an inline comment as done.

Address comments.

hokein marked an inline comment as done.Feb 6 2019, 7:19 AM
hokein updated this revision to Diff 185551.Feb 6 2019, 7:20 AM

Remove accident change.

Harbormaster completed remote builds in B27819: Diff 185551.
hokein closed this revision.Feb 7 2019, 1:41 AM

Committed in rL353306.

Could we move the code to the Tweak itself, so that all the callers would get the formatting? I.e.

class Tweak {
   Replacements apply() {  // This is now non-virtual.
     auto Replacements = doApply();
     return cleanupAndFormat(Replacements);
   }

protected:
   virtual Replacements doApply() = 0; // inheritors should implement this now. Note: the name is terrible, suggestions are welcome.
};

This would ensure the callers don't need to deal with the formatting on their own.

This seems introduce intrusive changes to the Tweak interface (Tweak will need to know the FormatStyle somehow), I think this might be done in another patch.

I agree with this concern, and don't think this is an appropriate layer to be aware of styling or failing on format errors. Can we consider moving the formatting to clangdserver as originally planned?

I agree with this concern, and don't think this is an appropriate layer to be aware of styling or failing on format errors. Can we consider moving the formatting to clangdserver as originally planned?

clang-format seems to be somewhat stable, do we actually expect that to be a large problem in practice?
On the other side, I can't imagine any clients that don't need formatting? E.g. it's nice when tests give the same results as one would see in the final clangd and tests don't go through ClangdServer.

I agree with this concern, and don't think this is an appropriate layer to be aware of styling or failing on format errors. Can we consider moving the formatting to clangdserver as originally planned?

clang-format seems to be somewhat stable, do we actually expect that to be a large problem in practice?
On the other side, I can't imagine any clients that don't need formatting? E.g. it's nice when tests give the same results as one would see in the final clangd and tests don't go through ClangdServer.

It's not about stability or whether the functionality is desired, but layering.
Unit tests having narrow scope is a good thing - if we want system tests that check clangdserver's behavior, they should test clangdserver.
Clients that don't go through clangdserver probably want formatting, but an immediate cleanupAndFormat on each generated change isn't necessarily the right way to do that.

It's not about stability or whether the functionality is desired, but layering.
Unit tests having narrow scope is a good thing - if we want system tests that check clangdserver's behavior, they should test clangdserver.
Clients that don't go through clangdserver probably want formatting, but an immediate cleanupAndFormat on each generated change isn't necessarily the right way to do that.

My argument is that it's better to provide formatting by default in the public interface for applying a single tweak.
I might be missing some use-cases, e.g. one that comes to mind is applying multiple tweaks in a row, in which case we'd want to format once and not for every tweak.

My concerns are code duplication and ease of use for the clients. Having both apply and applyAndFormat (the latter being a non-virtual or a free-standing utility function) in the public interface of the Tweak would totally do it for me.
However, I also think tests should format by default, not sure we agree on this.
WDYT?

It's not about stability or whether the functionality is desired, but layering.
Unit tests having narrow scope is a good thing - if we want system tests that check clangdserver's behavior, they should test clangdserver.
Clients that don't go through clangdserver probably want formatting, but an immediate cleanupAndFormat on each generated change isn't necessarily the right way to do that.

My argument is that it's better to provide formatting by default in the public interface for applying a single tweak.
I might be missing some use-cases, e.g. one that comes to mind is applying multiple tweaks in a row, in which case we'd want to format once and not for every tweak.

If providing formatting was free, I'd be fine with this, but it leaks into the public interface in two places: a) it requires the caller to plumb through a formatting style, and b) it is another source of errors.

For this particular interface it's more important that it's conceptually clear and easy to implement than it is that it's easy to call - this is an extension point.

My concerns are code duplication and ease of use for the clients. Having both apply and applyAndFormat (the latter being a non-virtual or a free-standing utility function) in the public interface of the Tweak would totally do it for me.

I'd be happy with applyAndFormat as a free function - my main concern is that formatting not be part of the scope of the class.

However, I also think tests should format by default, not sure we agree on this.
WDYT?

I'd rather they didn't format, but I don't think it will matter much and I'm happy either way.

(Where it does matter, I'd rather have the differences between input/output be a result of the tweak replacements, not of different formatting triggered by a different token sequence.
I don't think there's much point in testing clang-format here, though we should have one test that verifies we're calling it at all.)

It's not about stability or whether the functionality is desired, but layering.
Unit tests having narrow scope is a good thing - if we want system tests that check clangdserver's behavior, they should test clangdserver.
Clients that don't go through clangdserver probably want formatting, but an immediate cleanupAndFormat on each generated change isn't necessarily the right way to do that.

My argument is that it's better to provide formatting by default in the public interface for applying a single tweak.
I might be missing some use-cases, e.g. one that comes to mind is applying multiple tweaks in a row, in which case we'd want to format once and not for every tweak.

If providing formatting was free, I'd be fine with this, but it leaks into the public interface in two places: a) it requires the caller to plumb through a formatting style, and b) it is another source of errors.

For this particular interface it's more important that it's conceptually clear and easy to implement than it is that it's easy to call - this is an extension point.

My concerns are code duplication and ease of use for the clients. Having both apply and applyAndFormat (the latter being a non-virtual or a free-standing utility function) in the public interface of the Tweak would totally do it for me.

I'd be happy with applyAndFormat as a free function - my main concern is that formatting not be part of the scope of the class.

Given that clangdServer is the only client of Tweaks (if we don't do format in tests), I think it is fine to move out applyAndFormat functionality from Tweaks. I somehow agree that applyAndFormat is a standalone function.

However, I also think tests should format by default, not sure we agree on this.
WDYT?

I'd rather they didn't format, but I don't think it will matter much and I'm happy either way.

(Where it does matter, I'd rather have the differences between input/output be a result of the tweak replacements, not of different formatting triggered by a different token sequence.
I don't think there's much point in testing clang-format here, though we should have one test that verifies we're calling it at all.)

Personally, I prefer not doing format in tests -- it would make writing testcases easier, it is annoying to spend time on figuring out format-only differences between actual code and expected code.