Re: [boost] [Boost-commit] svn:boost r81360 - in trunk: boost/algorithm libs/algorithm/test

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