[squid-dev] [PATCH] SBuf const iterator fixes

[Amos Jeffries]

On 9/02/2016 8:38 a.m., Kinkie wrote:
> On Mon, Feb 8, 2016 at 8:15 PM, Alex Rousskov
> <rousskov at measurement-factory.com> wrote:
>> On 02/08/2016 11:18 AM, Amos Jeffries wrote:
>>> The SBufIterator is mostly actually a const iterator. But not quite.
>>
>> I am not sure I understand what you are implying because you did not
>> change any const-related aspects of SBufIterator. What are the
>> requirements for being a "const iterator" class that old SBufIterator
>> violated?
> 
> It behaves like a const while not being explicitly so.
> You can't do
> auto i = sbuf.begin();
> *i = 'a';
> 
>>> The point of difference from const_iterator is the operator*() API which
>>> is also different from the normal itertor API in that is returns a char
>>> instead of a char& or const char &.
>>
>> Please explain why returning char is wrong for const_iterator. I
>> understand that [all?] standard iterators return a reference, and I also
>> understand that a non-const iterator cannot return a char, but I do not
>> yet understand how that difference affects constant SBufIterator.
> 
> It behaves the same,but it's a different API.

With some auto-magic differences resulting from that.

The char return is a temporary copy that the compiler can apply move
semantics to. Whereas a reference is not - new objects from it must be
copy-constructed.

Also, non-const iterators can have reference taken to the iter[] member
and not to the 'stack' temporary - if anything were actually using
iterators that way we would get into trouble.

eg. roughly like so (this fails only due to our c_str())

  auto itr = string.begin()
  char *foo = &*itr;
  assert(strcmp(foo, string.c_str()) == 0);



> 
>> Overall, it feels like returning a simple "char" may be more efficient
>> than forcing the compiler to optimize away references to "char", but I
>> may be missing something important here.
> 
> WRT the const iterator, yes. But the iterator should return a char
> referente, and COW on access which is tricky.

And as it turns out we dont need non-const iterator at this point. So I
have not even bothered to make this implement both. Just adjusted it to
an STL consistent API, and called it such.


> 
>>> This has side effects on the reverse iterator and SBuf begin/end
>>> methods. Which are also const'ified.
>>
>> Please note that in the C++11 land, we are supposed to use
>> cbegin/cend/etc. methods if we unconditionally want a const_iterator.
>> These are added to the old set of begin/end/etc. methods (half of those
>> old methods return const_iterators, depending on the object constness
>> rather than method name):
> 
> nod. so far we are only reading from iterators.


For some reason the GCC range-for loop requires begin()/end() for its
expansion. Even when "const auto" is used explicitly.

"
tests/testRFC3986.cc:60:5:   required from here
anyp/Rfc3986.h:67:5: error: ‘begin’ was not declared in this scope
     for (const auto c : s) {
     ^
"


So I have just converted the begin/end methods of SBuf to be the C++11
const form, omitting the non-const and c*() variants.

If you would like the c*() methods added I am okay with doing that on
commit. We just dont use them, so it seems pointless extra code right now.

Amos