Page MenuHomePhabricator

Add support to llvm::Twine for storing formatv objects
ClosedPublic

Authored by zturner on Dec 15 2016, 5:11 PM.

Details

Summary

Given a function foo(const Twine &T), it would be nice to be able to write foo(formatv("{0}{1}", x, y));. This patch enables that.

For some reason there's some formatting-only changes in here. I suspect this is due to an incompatibility between mono-repo and the git clang-format extension script. I'll try to clean these up before submitting.

Diff Detail

Repository
rL LLVM

Event Timeline

zturner updated this revision to Diff 81691.Dec 15 2016, 5:11 PM
zturner retitled this revision from to Add support to llvm::Twine for storing formatv objects.
zturner updated this object.
zturner added reviewers: dblaikie, chandlerc, beanz.
zturner added a subscriber: llvm-commits.

Twine.h is all screwed up (upstream) so I'm going to have to figure this out and resubmit the patch so the formatting isn't messed up. Be back later with a fixed patch

zturner updated this revision to Diff 81695.Dec 15 2016, 5:42 PM

Fixed patch, hopefully it's better this time.

dblaikie edited edge metadata.Dec 15 2016, 6:15 PM

Could you explain what makes each of the test cases interesting? (what they're testing that other test cases aren't) - it seems like a bit more testing than I'd expect, but I'm probably missing something about the mechanics of Twine.

Also - would it be worth adding a test case to demonstrate that it truly is lazy? If you introduce a type with a custom printer that has a side effect (such as modifying the object as it prints) - then create a Twine of a formatv, observe the side effect hasn't occurred, then stringify the Twine and observe that the side effect has occurred.

zturner updated this revision to Diff 81768.Dec 16 2016, 9:38 AM
zturner edited edge metadata.

Removed a redundant test case and added a test case to verify laziness.

dblaikie accepted this revision.Dec 16 2016, 9:44 AM
dblaikie edited edge metadata.
dblaikie added inline comments.
llvm/unittests/ADT/TwineTest.cpp
105 ↗(On Diff #81768)

public inheritance is the default for structs, so you can omit it if youl ike

111–112 ↗(On Diff #81768)

I'd probably skip these helper functions and the expressions below can just be:

Twine(formatv(...));

and

Twine(formatv(...)).str();

(possibly with (void) casts for documentation)

118 ↗(On Diff #81768)

Either you can access the local (non-member) Count directly here, or you could probably simplify Formatter and just have it take no ctor argument and initialize the Count member to zero instead? (or is the Formatter copied into the formatv call? In which case I'd probably just access 'Count' directly as the local variable here)

119–122 ↗(On Diff #81768)

What's the difference between these two cases? Demonstrating that its reentrant/non-caching?

This revision is now accepted and ready to land.Dec 16 2016, 9:44 AM
zturner added inline comments.Dec 16 2016, 9:49 AM
llvm/unittests/ADT/TwineTest.cpp
118 ↗(On Diff #81768)

Yes the formatter gets copied in the formatv call. Accessing the local count variable seems reasonable.

119–122 ↗(On Diff #81768)

I suppose there isn't much. I can remove the second one.

This revision was automatically updated to reflect the committed changes.