Skip to main content
Commonmark migration
Source Link

#Implementation

Implementation

  • operator==(SplitStringIterator const&) only compares pointers for equality, not lengths. This might not be intended.
  • The ForwardIterator concept isn't actually fulfilled: For that, operator* has to return a reference! (And it makes sense, too: You cannot provide a multipass guarantee in case the contents get modified, i.e. a previously different CharT value gets changed to delim.)
  • Why restrict delim to CharT, when it could just as easily be a std::basic_string_view<CharT>? The advance_() logic would have to be updated for the varying length, but that's the only non-type change I can see that would be necessary. (Though one could add a conversion or a special case for single CharT delimiters.)

Small caveat: If parts_indexable had a longer lifetime than data (e.g. it got returned by foo), all its contents would dangle (though this is inherent problem of std::string_view, and not of your implementation). This could be fixed by making explicit copies by changing the type of parts_indexable to std::vector<std::string>. (Not a criticism of your work, just a note for users of this iterator).

#Implementation

  • operator==(SplitStringIterator const&) only compares pointers for equality, not lengths. This might not be intended.
  • The ForwardIterator concept isn't actually fulfilled: For that, operator* has to return a reference! (And it makes sense, too: You cannot provide a multipass guarantee in case the contents get modified, i.e. a previously different CharT value gets changed to delim.)
  • Why restrict delim to CharT, when it could just as easily be a std::basic_string_view<CharT>? The advance_() logic would have to be updated for the varying length, but that's the only non-type change I can see that would be necessary. (Though one could add a conversion or a special case for single CharT delimiters.)

Small caveat: If parts_indexable had a longer lifetime than data (e.g. it got returned by foo), all its contents would dangle (though this is inherent problem of std::string_view, and not of your implementation). This could be fixed by making explicit copies by changing the type of parts_indexable to std::vector<std::string>. (Not a criticism of your work, just a note for users of this iterator).

Implementation

  • operator==(SplitStringIterator const&) only compares pointers for equality, not lengths. This might not be intended.
  • The ForwardIterator concept isn't actually fulfilled: For that, operator* has to return a reference! (And it makes sense, too: You cannot provide a multipass guarantee in case the contents get modified, i.e. a previously different CharT value gets changed to delim.)
  • Why restrict delim to CharT, when it could just as easily be a std::basic_string_view<CharT>? The advance_() logic would have to be updated for the varying length, but that's the only non-type change I can see that would be necessary. (Though one could add a conversion or a special case for single CharT delimiters.)

Small caveat: If parts_indexable had a longer lifetime than data (e.g. it got returned by foo), all its contents would dangle (though this is inherent problem of std::string_view, and not of your implementation). This could be fixed by making explicit copies by changing the type of parts_indexable to std::vector<std::string>. (Not a criticism of your work, just a note for users of this iterator).

Source Link
hoffmale
  • 6.5k
  • 17
  • 41

#Implementation

  • operator==(SplitStringIterator const&) only compares pointers for equality, not lengths. This might not be intended.
  • The ForwardIterator concept isn't actually fulfilled: For that, operator* has to return a reference! (And it makes sense, too: You cannot provide a multipass guarantee in case the contents get modified, i.e. a previously different CharT value gets changed to delim.)
  • Why restrict delim to CharT, when it could just as easily be a std::basic_string_view<CharT>? The advance_() logic would have to be updated for the varying length, but that's the only non-type change I can see that would be necessary. (Though one could add a conversion or a special case for single CharT delimiters.)

Small caveat: If parts_indexable had a longer lifetime than data (e.g. it got returned by foo), all its contents would dangle (though this is inherent problem of std::string_view, and not of your implementation). This could be fixed by making explicit copies by changing the type of parts_indexable to std::vector<std::string>. (Not a criticism of your work, just a note for users of this iterator).