[squid-dev] [PATCH] PackableStream for cachemgr reports

[Alex Rousskov]

On 08/09/2015 01:47 AM, Amos Jeffries wrote:
>>> However the char(int_type) type constructor is necessary because this is
>>> the fundamental int_type being passed in. All the available
>>> *_cast<char>() operators are built to convert a 32-bit int_type value
>>> into an array of 4 bytes before dealing with the byte->char conversion
>>> part. Which then incorrectly takes the lowest-8 bits rather than the
>>> first-8.
>>
>> I am curious where you got the above information from, especially the
>> last sentence.


> From several long days testing why static_cast always 0-terminated the
> strings when trying to fix this same thing in the same way for
> StoreEntryStream years back. LP says rev.11605 (initial c++0x support).

Trunk r11605 just enables C++0x. The commit message does not mention how
that relates to the code change we are discussing. Are you saying that
enabling C++0x support broke the code with implicit casts [and did not
let you add an explicit static_cast]?


> Repeating the tests it seems to work now. Although I am building with
> GCC-5 now. IIRC that was about GCC 4.6-ish.

My simple static_cast tests appear to work as expected with GCC 4.4, but
I do not know how to reproduce the problem you were facing. GCC 4.4
defines "int_type" for char-based traits as "int", just like the modern
GCC versions do and the STL documentation says.


> +            // NP: cast because GCC promotes int_type to 32-bit type
> +            //     std::basic_streambuf<char>::int_type {aka int}
> +            //     despite the definition with 8-bit type value.

If you are sure that static_cast did not work for earlier GCC versions,
please s/GCC/old GCC/ or something like that in your comment because the
comment does not make sense in the context of the [current] GCC STL code
and the C++ standard itself.

What definition do you refer to as "definition with 8-bit type value"?

How can "GCC promote int_type to 32-bit type ... int" if int_type _is_
int (in both traits and streambuf)?


>> Also, do not declare "chars" until you need them. This is unrelated to
>> casting. It is related to where you declare "chars". You are declaring
>> them too early, inserting them in the wider context where they are not
>> needed AFAICT.


> Looking up the origins of the hack I found the chars being outside the
> if() was result of a different patch by one Alex R when adding the if(). :-P

If you mean trunk r7557, that was not my code, but even if it were, I am
surprised you waste your time on investigating who committed imperfect
code almost ten years ago and then sharing that invaluable information
with the rest of us.

If you want to accuse me of being more picky about others code, then
please say so explicitly (but it does not address the problem at hand).
If you want to keep the new class in sync with the old code, then please
use a source code comment to document your decision. If you want to
excuse minor problems in the code you propose, then I recommend just
fixing them instead because doing so saves time.


> +        if (aChar != traits_type::eof()) {
...
> +            if (aChar != traits_type::eof())

And why test the same thing twice? The aChar variable does not change.
Can we just remove the second test?


>>>>> +                outBuf->append(chars, 1);
>>>> ...
>>>>> +            outBuf->append(pbase(), pending);
>>>> ...
>>>>> +            outBuf->append(chars, number);
>>>>
>>>> To reduce code duplication and to assist with debugging, it would be
>>>> nice to move these three into a single dedicated private write() method
>>>> or similar.
>>>>
>>>
>>> Calling xsputn() instead now. Although note that trampolining off a
>>> virtual function (non-inlinable in some compilers) actually makes it
>>> less efficient.
>>
>>
>> This is exactly the reason I did not suggest to call xsputn(). Please do
>> not call that virtual method! Please add and call a dedicated private
>> write() method (or similar).
> 
> So what is the point then?

I have documented both points in my original request: To reduce code
duplication and to assist with debugging.


> If it is inline the compiler optimizes it away even in stack traces. No
> use for debugging then.

Unless one builds with inlining disabled (as some of us often do) or
adds a temporary debugs() statement to that new method.

Alex.