Page MenuHomePhabricator

Add llvm::StringLiteral
ClosedPublic

Authored by zturner on Dec 12 2016, 2:15 PM.

Details

Summary

As discussed on the mailing list, this allows us to create global tables of StringRefs that don't incur a global constructor.

Diff Detail

Repository
rL LLVM

Event Timeline

zturner updated this revision to Diff 81138.Dec 12 2016, 2:15 PM
zturner retitled this revision from to Add llvm::StringLiteral.
zturner updated this object.
zturner added a subscriber: llvm-commits.
yaron.keren added inline comments.
include/llvm/ADT/StringRef.h
839 ↗(On Diff #81138)

wrapper around StringRef

zturner added a comment.EditedDec 12 2016, 4:00 PM
This comment has been deleted.
include/llvm/ADT/StringRef.h
839 ↗(On Diff #81138)

I'm open to better wording here, but I don't think this is accurate either. A "wrapper around a StringRef" implies that you need a StringRef in order to create one of these. But it's exactly the opposite, you really do need a string literal (i.e. object of the form "abcdefg") to create one of these.

mehdi_amini added inline comments.Dec 12 2016, 6:18 PM
include/llvm/ADT/StringRef.h
90 ↗(On Diff #81138)

Too bad we're losing the assert, but I see why. An alternative is another (possibly protected) ctor that would be constexpr and used by StringLiteral

839 ↗(On Diff #81138)

Nit, from the coding standards: Don’t duplicate function or class name at the beginning of the comment. For humans it is obvious which function or class is being documented; automatic documentation processing tools are smart enough to bind the comment to the correct declaration.

(Also technically it is more of a wrapper around a string literal more than around a StringLiteral)

843 ↗(On Diff #81138)

May add a warning about null in the middle of a literal? (That's a behavior change from StringRef)

mehdi_amini accepted this revision.Dec 12 2016, 6:31 PM
mehdi_amini edited edge metadata.

(Otherwise LGTM)

This revision is now accepted and ready to land.Dec 12 2016, 6:31 PM
efriedma added inline comments.
include/llvm/ADT/StringRef.h
90 ↗(On Diff #81138)

Stupid trick to save the assert:

/*implicit*/ constexpr StringRef(const char *data, long length)
    : Data(data), Length((data || length == 0) ? length : (assert(0 && "Bad StringRef"), 0)) { }
zturner added inline comments.Dec 12 2016, 8:10 PM
include/llvm/ADT/StringRef.h
90 ↗(On Diff #81138)

Won't this still complain that assert is not a constant expression and thus can't be used in a constexpr function?

malcolm.parsons requested changes to this revision.Dec 12 2016, 11:53 PM
malcolm.parsons edited edge metadata.
malcolm.parsons added inline comments.
unittests/ADT/StringRefTest.cpp
1007 ↗(On Diff #81138)

There is no test for the length of the StrlingLiteral.
Maybe EXPECT_EQ(StringRef("Foo"), Strings[0]);

This revision now requires changes to proceed.Dec 12 2016, 11:53 PM
This revision was automatically updated to reflect the committed changes.
efriedma added inline comments.Dec 13 2016, 10:37 AM
include/llvm/ADT/StringRef.h
90 ↗(On Diff #81138)

No... formally, [expr.const] in the C++11 standard says that when checking whether an expression is a constant expression, "conditional operations that are not evaluated are not considered". In practice, this means you'll get an error at compile time only if the condition is false, which is exactly the behavior you want.

I actually screwed up the implementation slightly; it's not a no-op if assertions are turned off, but that's easy to fix:

/*implicit*/ constexpr StringRef(const char *data, long length)
    : Data(data), Length((data || length == 0) ? length : (assert(0 && "Bad StringRef"), length)) { }