This is an archive of the discontinued LLVM Phabricator instance.

[SmallString] Add explicit conversion to std::string
ClosedPublic

Authored by JDevlieghere on Jan 29 2020, 9:34 AM.

Details

Summary

With the conversion between StringRef and std::string now being explicit, converting SmallStrings becomes more tedious. This patch adds an explicit operator so you can write std::string(Str) instead of Str.str().str().

Diff Detail

Event Timeline

JDevlieghere created this revision.Jan 29 2020, 9:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 29 2020, 9:34 AM
Herald added a subscriber: dexonsmith. · View Herald Transcript
MaskRay accepted this revision.Jan 29 2020, 10:12 AM
MaskRay added inline comments.
llvm/include/llvm/ADT/SmallString.h
278

The comment is redundant, because the code self explains.

This revision is now accepted and ready to land.Jan 29 2020, 10:12 AM
JDevlieghere marked 2 inline comments as done.Jan 29 2020, 10:14 AM
JDevlieghere added inline comments.
llvm/include/llvm/ADT/SmallString.h
278

It's the same in StringRef, but I agree!

Unit tests: fail. 62303 tests passed, 1 failed and 838 were skipped.

failed: libc++.std/thread/thread_mutex/thread_mutex_requirements/thread_mutex_requirements_mutex/thread_mutex_recursive/try_lock.pass.cpp

clang-tidy: fail. clang-tidy found 0 errors and 4 warnings. 0 of them are added as review comments below (why?).

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

With the conversion between StringRef and std::string now being explicit, converting SmallStrings becomes more tedious

Explicitly mention 777180a32b61070a10dd330b4f038bf24e916af1 in the description.

This revision was automatically updated to reflect the committed changes.
JDevlieghere marked an inline comment as done.
cishida reopened this revision.Jan 29 2020, 1:35 PM

This indirection seems like unnecessary work. Could you reimplement this as a direct conversion to std::string?

This revision is now accepted and ready to land.Jan 29 2020, 1:35 PM

This indirection seems like unnecessary work. Could you reimplement this as a direct conversion to std::string?

Good catch, thank you!

This revision was automatically updated to reflect the committed changes.
craig.topper added inline comments.
llvm/include/llvm/ADT/SmallString.h
279

I'm late here, but that should probably be this->data() instead of this->begin(). It does the same thing in this case, but begin() returns an "iterator" while data() returns a pointer. And the constructor that's being called is defined as taking a pointer and size.

JDevlieghere marked 2 inline comments as done.Jan 30 2020, 9:06 PM
JDevlieghere added inline comments.
llvm/include/llvm/ADT/SmallString.h
279

Thanks Craig! I've fixed it for the std::string and llvm::StringRef case above.

To github.com:llvm/llvm-project.git
   cfebd777422..a5f479473b2  master -> master