This is an archive of the discontinued LLVM Phabricator instance.

Add ctor for string literal to StringRef, and make explicit the conversion from const char *
AbandonedPublic

Authored by mehdi_amini on Oct 14 2016, 6:07 PM.

Details

Summary

The implicit conversion from const char * to StringRef involve calling strlen.
Having "expensive" calls happening implicitly with a class like StringRef which
is supposed to be a lightweight wrapper does not seem like a good idea to me.
I already landed over 30 patches converting multiple APIs to use StringRef, this
is the last one.

Making the conversion explicit allows to have a constexpr constructor for string
literals (or literal char arrays), making global tables of StringRef possible
without needing a static initializer.

After this series of patches, an LTO link of llvm-tblgen is calling ~1.5M times
strlen() vs over 11M before.

A frequent pattern I found that making this constructor explicit helps avoiding
is a conversion from std::string or StringRef to const char * back to
StringRef, often in a single expression. For Example:

llvm::SmallString<256> CleanPath(Entry.first.data());

Removing the .data() avoids a call to strlen.

Most of the call sites here have been updated with a simple clang-tidy check,
with some manual tweaking when needed.

Here is the check if anyone needs to update some codebase downstream:

void StringrefCheck::registerMatchers(MatchFinder *Finder) {

Finder->addMatcher(
    materializeTemporaryExpr(
        hasType(namedDecl(hasName("llvm::StringRef"))),
        has(expr(ignoringImpCasts(cxxConstructExpr(
            argumentCountIs(1),
            hasArgument(0, expr(hasType(pointsTo(isAnyCharacter())))))))))
        .bind("imp_conv"),
    this);

}
void StringrefCheck::check(const MatchFinder::MatchResult &Result) {

const auto *MatchedCtor =
    Result.Nodes.getNodeAs<MaterializeTemporaryExpr>("imp_conv");
const auto FixCondStart = FixItHint::CreateInsertion(
    MatchedCtor->getLocStart(), "llvm::StringRef(");
const auto FixCondEnd = FixItHint::CreateInsertion(
    Lexer::getLocForEndOfToken(MatchedCtor->getLocEnd(), 0,
                               *Result.SourceManager,
                               Result.Context->getLangOpts()),
    ")");

auto Diag = diag(MatchedCtor->getExprLoc(), "Explicit StringRef ctor:");
Diag << FixCondStart << FixCondEnd;

}

Diff Detail

Event Timeline

mehdi_amini retitled this revision from to Add ctor for string literal to StringRef, and make explicit the conversion from const char *.
mehdi_amini updated this object.
mehdi_amini added reviewers: dexonsmith, zturner, pete.
mehdi_amini added a subscriber: llvm-commits.

Note: the patch is without context because otherwise it is too large for Phab.

You might like to take a look at the ConstCharArrayDetector stuff in the LibreOffice codebase here:

https://cgit.freedesktop.org/libreoffice/core/tree/include/rtl/ustring.hxx

where Stephan Bergman has implemented optimisations to avoid calling strlen() at runtime when dealing with static const char arrays.

As far as I know, clang will compute strlen() at compile time if it knows the string being passed is a static constant

malcolm.parsons edited edge metadata.
zturner edited edge metadata.Oct 16 2016, 8:42 PM

The only thing I'm not crazy about here is that the clang tidy check seems to blindly replace all calls to s.c_str() with llvm::StringRef(s.c_str()). Is there any way to make it attempt to replace it with just s first, and only if that fails do you then try llvm::StringRef(s.c_str())?

"Maybe"?
(I've never wrote anything with clang-tidy before, so I'm not sure I could do it by myself right now)

The only thing I'm not crazy about here is that the clang tidy check seems to blindly replace all calls to s.c_str() with llvm::StringRef(s.c_str()). Is there any way to make it attempt to replace it with just s first, and only if that fails do you then try llvm::StringRef(s.c_str())?

Shouldn't it be relatively straight forward to discover whether the expression is convertible to StringRef without the .c_str() call from an AST matcher?

mehdi_amini abandoned this revision.Oct 27 2016, 2:17 PM

Abandon the changes to StringRef.