Subject: Re: [boost] [Boost-commit] svn:boost r81360 - in trunk: boost/algorithm libs/algorithm/test
From: Marshall Clow (mclow.lists_at_[hidden])
Date: 2012-11-15 17:13:44
On Nov 15, 2012, at 1:32 PM, Steven Watanabe <watanabesj_at_[hidden]> wrote:
> AMDG
Thanks for the review!
> On 11/15/2012 11:45 AM, marshall_at_[hidden] wrote:
>> <snip>
>> + // construct/copy
>> + BOOST_CONSTEXPR basic_string_ref ()
>> +#ifdef BOOST_NO_CXX11_NULLPTR
>> + : ptr_(NULL), len_(0) {}
>> +#else
>> + : ptr_(nullptr), len_(0) {}
>> +#endif
>
> Really? The two branches have exactly the same
> effect. The only reason to prefer nullptr is aesthetic
> and this is more than offset by the #ifdef.
Yeah.
>
>> +
>> + basic_string_ref(const charT* str)
>> + : ptr_(str), len_(std::strlen(str)) {}
>> +
>
> strlen only works for char. You need to use traits::length
Fixed.
>
>> +#ifndef BOOST_NO_CXX11_HDR_INITIALIZER_LIST
>> +// !! How do I do this? Look how initializer_lists work!
>> + basic_string_ref(std::initializer_list<charT> il); // TODO
>> +#endif
>> +
>
> Although this can be implemented, it's incredibly dangerous,
> because the string_ref will be left dangling when the
> initializer_list goes out of scope.
Still there.
>>
>> + // iterators
>> + BOOST_CONSTEXPR const_iterator begin() const { return ptr_; }
>> + BOOST_CONSTEXPR const_iterator cbegin() const { return ptr_; }
>> + BOOST_CONSTEXPR const_iterator end() const { return ptr_ + len_; }
>> + BOOST_CONSTEXPR const_iterator cend() const { return ptr_ + len_; }
>> + const_reverse_iterator rbegin() const { return const_reverse_iterator (end()); }
>> + const_reverse_iterator crbegin() const { return const_reverse_iterator (end()); }
>> + const_reverse_iterator rend() const { return const_reverse_iterator (begin()); }
>> + const_reverse_iterator crend() const { return const_reverse_iterator (begin()); }
>> +
>
> Check for tabs?
When I de-tabbed it, it got messed up.
>> + // capacity
>> + BOOST_CONSTEXPR size_type max_size() const { return len_; }
>
> I'm not convinced that this definition is correct.
> Returns: the size of the largest possible string.
>
> Note that this does not say that it returns the
> maximum size that this particular string_ref object
> can hold without reassignment.
I'll think about this.
>>
>> + int compare(basic_string_ref x) const {
>> + int cmp = std::memcmp ( ptr_, x.ptr_, std::min(len_, x.len_));
>> + return cmp != 0 ? cmp : ( len_ == x.len_ ? 0 : len_ < x.len_ ? -1 : 1 );
>> + }
>> +
>> + bool starts_with(charT c) const { return !empty() && front() == c; }
>> + bool starts_with(basic_string_ref x) const {
>> + return len_ >= x.len_ && std::memcmp ( ptr_, x.ptr_, x.len_ ) == 0;
>> + }
>> +
>> + bool ends_with(charT c) const { return !empty() && back() == c; }
>> + bool ends_with(basic_string_ref x) const {
>> + return len_ >= x.len_ && std::memcmp ( ptr_ + len_ - x.len_, x.ptr_, x.len_ ) == 0;
>> + }
>> +
>
> You can only use memcmp for char. You have to use
> traits::compare here.
Fixed.
>> +// Have to use traits here
>> + size_type find(basic_string_ref s) const {
>> + const_iterator iter = std::find_if ( this->cbegin (), this->cend (),
>> + s.cbegin (), s.cend (), traits::eq );
>> + return iter = this->cend () ? npos : std::distance ( this->cbegin (), iter );
>> + }
>> +
>
> I think you mean std::search, not std::find_if.
D'oh!
>> + size_type find(charT c) const {
>> +#ifdef BOOST_NO_CXX11_LAMBDAS
>> + const_iterator iter = std::find_if ( this->cbegin (), this->cend (),
>> + detail::string_ref_traits_eq<charT, traits> ( c ));
>> +#else
>> + const_iterator iter = std::find_if ( this->cbegin (), this->cend (),
>> + [c] ( charT val ) { return traits::eq ( c, val ); } );
>> +#endif
>> + return iter == this->cend () ? npos : std::distance ( this->cbegin (), iter );
>> + }
>> +
>
> As with NULL/nulptr, I don't see the point of using
> C++11 lambdas if you have to #ifdef it with a C++03
> implementation.
Thinking about this...
-- Marshall
Marshall Clow Idio Software <mailto:mclow.lists_at_[hidden]>
A.D. 1517: Martin Luther nails his 95 Theses to the church door and is promptly moderated down to (-1, Flamebait).
-- Yu Suzuki