Re: stringng-cherrypick r12764

On Fri, Sep 27, 2013 at 12:11 AM, Alex Rousskov
<rousskov_at_measurement-factory.com> wrote:
> On 09/26/2013 01:43 PM, Kinkie wrote:
>> On Mon, Aug 26, 2013 at 4:22 AM, Alex Rousskov wrote:
>
>>>> + * SBuf.cc (C) 2008 Francesco Chemolli <kinkie_at_squid-cache.org>
>>>
>>> If you do not mind, please drop the copyright statements (here and in
>>> other files you are adding). You are not the only person contributing
>>> this code, and managing per-file copyright claims correctly is
>>> practically impossible (incomplete copyright information in your files
>>> is a case in point).
>
>> That would apply to the whole Squid source tree (there's plenty of
>> files which carry copyright notices by individual contributors).
>
> You are correct, and I have acknowledged that this is an old problem in
> my request.
>
>
>> But this is a matter of copyright policy (and possible assignment
>> thereof) which should probably be tackled at the Foundation level.
>
> Yes, but you can help by not making the situation slightly worse.
>
>
>> Among other questions, once I remove this, what copyright statement
>> should I put?
>
> None.
>
>
>> Wouldn't then be automatically be reassigned to
>> the only entity named in the COPYRIGHT file, University of California?
>
> No. Copyright does not depend on a C++ comment you put (or do not put)
> in a source file. Putting a misleading comment just makes it harder to
> track the owners of the code correctly.
>
>
>> It can be argued that the per-file copyright belongs to the main
>> contributor, while each additional contributor has copyright over
>> their respective contributions. This would seem to be in line with
>> some past contributions...
>
> While we can argue about it, the simple truth is that (a) you might not
> own the copyright for some bits of the code you want to commit and (b)
> you will most likely not own the copyright for some of the bits of the
> code that gets added after you commit. That is why I am not going to
> waste your time debating the fine points of copyright law. I already
> regret asking you to drop that line.

You shouldn't :)
You make fine points. I'd just love that the point would be clarified
ASAP in a policy statement.
I'd love to participate in the discussion on it, when the time will come.

>> This is too messy to address here, it'd be however nice if there was a
>> guideline.
>
> There will be (it is on the to-do list), and I hope it will be
> consistent with my unofficial request to remove that line. Removing that
> misleading line is easy and does not increase the mess, but, as I said,
> if you feel strongly about keeping it, do.

I'm removing it. I'm quite confident about the originality of my
contributions, and of course there are reasons why I would like to
keep my name attached to my contributions, but that can be done in
many ways and this is not the time to discuss about that.

>>> please do not forget that you wanted to make (or consider
>>> making?) size_type unsigned once everybody is happy with the code (but>>> before merging it).
>>
>>
>> Yes, that's the planned next step.
>> It seems to be working, however there is an annoyance when someone
>> passes a negative integer,
>
> That would be not just an annoyance but a bug in caller's code, right?
> In other words, no caller should pass negative integers as an
> SBuf::size_type method parameter before or after this change (with the
> exception of npos before the change, of course).

Yes, it is a bug in the caller code; at the same time it'd be nice to
catch it as clearly as possible rather than by not finding a pattern
which is there (negative-int becomes very-large-int which is obviously
bigger then length() which means guaranteed "not found" answer).

>> which gets automatically cast into
>> veryLargeInteger unless compile-time countermeasures are taken against
>> automatic type conversion (I've used the technique suggested here:
>> http://stackoverflow.com/questions/175689/can-you-use-keyword-explicit-to-prevent-automatic-conversion-of-method-parameter);
>> if this gets done, any integer literal must be explicitly qualified as
>> unsigned.
>
> That approach seems like worth trying. You may want to name the
> templated parameter wrongType or something like that, to clarify the
> intent to those who will get compiler errors when writing new code.
>
> BTW, don't some compilers issue warnings when signed is being converted
> to unsigned? Do those warnings cover method arguments? If so, we should
> be able to detect conversion bugs at test farm compile time even without
> this approach (not perfect, but better than nothing if we cannot use
> that trick).

g++ seems to support -Wconversion and -Wsign-conversion which are not
part of our current arguments set. It may be interesting to add them
(along maybe with -Wextra ? )
Maybe Amos has already done some work on that?

>> Currently being tested at lp:~squid/squid/stringng (without the
>> -cherrypick). If everyone is ok with this approach, I'm implementing
>> it throughout.
>
> I am worried that you would have to add a dozen or so templated method
> declarations. It feels like a bit too much for the problem at hand, but
> the only two specific concerns I have are:

Yes.

> * What it would do to compiler error messages, especially when some of
> the templated methods have the same name.

Here is the error message for the current experiment:

../../src/SBuf.h: In member function ‘void testSBuf::testFindChar()’:
../../src/SBuf.h:609: error: ‘template<class T> uint32_t
SBuf::find(char, T) const’ is private
../../src/tests/testSBuf.cc:468: error: within this context

> * What it would do to some less capable and/or experimental compilers
> that might get confused and start warning us.

I believe this should be a quite consolidated part of the standard,
dealing with automatic type promotions.

> These are minor and can be easily addressed by removing templated
> methods if the whole approach is deemed not worth the trouble after all.

It can probably be avoided by using those extra warning arguments I
mentioned above. I'm running a test now to see how it fares.

-- 
    /kinkie