This is an archive of the discontinued LLVM Phabricator instance.

[llvm] [ADT] Add a range/iterator-based Split()
ClosedPublic

Authored by mgorny on Sep 26 2021, 6:07 AM.

Details

Summary

Add a llvm::Split() implementation that can be used via range-for loop,
e.g.:

for (StringRef x : llvm::Split("foo,bar,baz", ','))
  ...

The implementation uses an additional SplittingIterator class that
uses StringRef::split() internally.

Diff Detail

Event Timeline

mgorny created this revision.Sep 26 2021, 6:07 AM
mgorny requested review of this revision.Sep 26 2021, 6:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 26 2021, 6:07 AM
mgorny added inline comments.Sep 26 2021, 6:07 AM
llvm/lib/IR/DataLayout.cpp
219

I needed to rename this due to collision with llvm::split class.

mgorny updated this revision to Diff 375126.Sep 26 2021, 12:36 PM

Convert more LLDB gdb-remote calls.

lattner accepted this revision.Sep 26 2021, 4:53 PM

This is really nice! Please fix the clang-tidy casing issues, but otherwise LGTM!

This revision is now accepted and ready to land.Sep 26 2021, 4:53 PM
mgorny updated this revision to Diff 375162.Sep 27 2021, 12:52 AM
mgorny retitled this revision from [llvm] [ADT] Add a range/iterator-based split() to [llvm] [ADT] Add a range/iterator-based Split().
mgorny edited the summary of this revision. (Show Details)

Attempt to fix linter issues.

This revision was landed with ongoing or failed builds.Sep 27 2021, 1:43 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 27 2021, 1:43 AM

I like it as well :)

llvm/include/llvm/ADT/StringExtras.h
519

This compares the contents of the StringRefs, while it should be comparing the pointers instead. I don't think it can cause correctness issues though I think it might be able to cause super-linear iteration complexity for some pathological inputs and more complex algorithms (a string consisting of many identical substrings, and an algorithm which compares two successive iterators).

Since comparing iterators from different sequences is UB, I think that comparing just Current.data() could actually suffice (the rest could be asserts) -- one has to be careful how he initializes the past-the-end iterator though.

527–529

std::tie(Current, Next) = ...

545–558

Could this be a function that returns iterator_range<SplittingIterator> ?
(I suppose lifetimes could be an issue, but it's not clear to me if that's the case here)

mgorny marked 3 inline comments as done.Sep 27 2021, 5:15 AM

Filed D110535 for suggested changes.

llvm/include/llvm/ADT/StringExtras.h
527–529

Damnit, I swear it didn't work before! Complained about this pointer something.