Page MenuHomePhabricator

clang-format: [JavaScript] Do requoting in a separate pass

Authored by djasper on Sep 7 2016, 3:41 PM.



The attempt to fix requoting behavior in r280487 after changes to tooling::Replacements are incomplete. We essentially need to add to replacements at the same position, one to insert a line break and one to change the quoting and that's incompatible with the new tooling::Replacement API, which does not allow for order-dependent Replacements. To make the order clear, Replacements::merge() has to be used, but that requires the merged Replacement to actually refer to the changed text, which is hard to reproduce for the requoting.

This change fixes the behavior by moving the requoting to a completely separate pass. The added benefit is that no weird ColumnWidth calculations are necessary anymore and this should just work even if we implement string literal splitting in the future.

Diff Detail

Event Timeline

djasper updated this revision to Diff 70611.Sep 7 2016, 3:41 PM
djasper retitled this revision from to clang-format: [JavaScript] Do requoting in a separate pass.
djasper updated this object.
djasper added a reviewer: mprobst.
djasper added a subscriber: cfe-commits.
mprobst added inline comments.Sep 7 2016, 3:43 PM

I guess we don't worry about the performance consequences here?

djasper added inline comments.Sep 7 2016, 3:45 PM

I don't think so. Lexing is very fast and we do the same thing to sort includes. We only create a new virtual environment if there actually are quotes to change which should be rare and also creating this environment should be fast.

All in all, I'd ignore performance here until this actually shows up in a profile (or we trace a slow formatting back to it).

mprobst accepted this revision.Sep 7 2016, 3:47 PM
mprobst edited edge metadata.
This revision is now accepted and ready to land.Sep 7 2016, 3:47 PM
djasper closed this revision.Sep 7 2016, 3:57 PM

Submitted as r280874.