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: Steven Watanabe (watanabesj_at_[hidden])
Date: 2012-11-15 16:32:38

AMDG

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.

> +            
> +        basic_string_ref(const charT* str)
> +            : ptr_(str), len_(std::strlen(str)) {}
> +

strlen only works for char.  You need to use traits::length

> +#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.

> +        
> +        // 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?

> +        // 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.

> +        
> +        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.

> +
> +//  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.

> +        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.

In Christ,
Steven Watanabe