This is an archive of the discontinued LLVM Phabricator instance.

Adding SmallString support to Twine
ClosedPublic

Authored by yaron.keren on Nov 22 2014, 10:34 AM.

Details

Summary

Teach Twine to support SmallString to save on .str() calls and later remove str() usage for these frequent cases.
Broke from http://reviews.llvm.org/D6336 for easier review.

Diff Detail

Event Timeline

yaron.keren updated this revision to Diff 16529.
yaron.keren retitled this revision from to Adding SmallString support to raw_ostream and Twine.
yaron.keren updated this object.
yaron.keren edited the test plan for this revision. (Show Details)
yaron.keren added subscribers: Unknown Object (MLST), Unknown Object (MLST), dbabokin, dexonsmith.

Correct missing empty line.

yaron.keren retitled this revision from Adding SmallString support to raw_ostream and Twine to Adding SmallString support to Twine.
yaron.keren updated this object.
yaron.keren edited the test plan for this revision. (Show Details)
yaron.keren added reviewers: dexonsmith, dblaikie.
yaron.keren edited the test plan for this revision. (Show Details)
yaron.keren removed subscribers: dexonsmith, dbabokin.

Rebased, added unit tests and some overloads to avoid ambiguity.

chfast added a subscriber: chfast.Mar 13 2015, 5:21 PM
chfast added inline comments.
include/llvm/MC/MCContext.h
236–238

If Twine can handle SmallString now, wouldn't it be better to reduce the number of overloads in this case?

yaron.keren added inline comments.Mar 14 2015, 9:31 AM
include/llvm/MC/MCContext.h
236–238

The preferred function is GetOrCreateSymbol(StringRef) in which Name directly indexes Symbols[]. GetOrCreateSymbol(Twine) is a slower convenience function, using a temporary buffer to print out the Twine into a SmallString and then call the StringRef version.

With MakeArgString the (StringRef) function is virtual and all other versions defer work to it.
I did notice MakeArgString(std::string Str) should take Str by reference and will update this.

Make MakeArgString and CopyString accept argument by reference.

chfast added inline comments.Mar 14 2015, 2:44 PM
lib/MC/MCContext.cpp
176–177

Do you think it is worth to check if Name isSingleStringRef?

Implemented Paweł suggestion to use isSingleStringRef() to reduce the number of overloads. GetOrCreateSymbol, CopyString and MakeArgString accept only Twine now, virtual MakeArgString(StringRef) is still required and was renamed to MakeArgStringRef to avoid ambiguity with the Twine version.

isSingleStringRef() is used through the toStringRef(), which was inlined to give the compiler a chance at optimizing out the Twine construction-isSingleStringRef testing-getSingleStringRef code sequence for simple arguments.

Removed function names from Twine.h comments.

chfast added inline comments.Mar 16 2015, 1:59 AM
tools/clang/lib/Sema/CodeCompleteConsumer.cpp
254 ↗(On Diff #22010)

I liked the argument name String better than T. I know the type has changed but CopyString copies strings.

Updated CopyString argument name to 'String'.

chfast accepted this revision.Mar 16 2015, 1:11 PM
chfast edited edge metadata.

LGTM

This revision is now accepted and ready to land.Mar 16 2015, 1:11 PM
yaron.keren closed this revision.Mar 17 2015, 2:54 AM

Committed revision 232465.