Re: [PATCH] Optimize HttpVersion comparison

On Tue, 24 Aug 2010 20:45:54 -0600, Alex Rousskov
<rousskov_at_measurement-factory.com> wrote:
> On 08/24/2010 08:27 PM, Amos Jeffries wrote:
>> On Tue, 24 Aug 2010 19:56:54 -0600, Alex Rousskov
>> <rousskov_at_measurement-factory.com> wrote:
>>> On 08/24/2010 07:25 PM, Henrik Nordström wrote:
>>>> tis 2010-08-24 klockan 19:15 -0600 skrev Alex Rousskov:
>>>>
>>>>> The current header sequence (somewhere) violates the squid-then-sys
>> rule
>>>>> and causes the problem. A header sequence that follows the
>>>>> squid-then-sys rule will not cause the problem. I suspect such
>> sequence
>>>>> does not exist (beyond one file scope) because some Squid headers
have
>>>>> to include system headers.
>>>>
>>>> And I say the opposite. The sequence that can fork is sys headers
first
>>>> then squid headers with #undef. The keywords are used both in the
squid
>>>> header and in squid code.
>>>
>>> Note that I was not arguing for one sequence or the other. I was just
>>> answering Amos' question.
>>
>> Okay. Thought I was going crazy there for a minute. The sequence is not
>> my
>> creation. AFAIK, it was designed explicitly to highlight such clashes
as
>> these and ensure the compiler warnings/errors point at the local copy
as
>> the faulty first-define which the dev at least has a hope of fixing
>> easily.
>>
>> The wiki documents it for .cc but leaves .h unstated, although the
>> minimal-symbols principal covers it for .h.
>>
>> I was planning to get someone to make a source format enforcer to check
>> the sequence was consistent. If we agree its a good or bad idea to keep
>> up
>> now is a good time to discuss that.
>>
>>>
>>>> squid heders first then sys headers renders the squid code using
these
>>>> members directly broken.
>>>
>>> In this particular case, with the compilers we know about, the squid
>>> code using these members directly works fine. This is because the
system
>>
>>> headers define a macro with a parameter:
>>>
>>> #define major(foo) gnu_craft_major_device(foo)
>>>
>>> The compilers we tested with do not substitute "major" unless it looks
>>> like a function call:
>>>
>>> http_ver.major = 0; // this is fine
>>> this->major< that.major; // this is fine too
>>> HttpVersion(): major(0) // this breaks
>>> HttpVersion(...): major(that.major) // this would break too
>>>
>>> This is why the problem is not visible until you try to polish the
>>> HttpVersion class.
>>>
>>> However, I would not be surprised if some other compilers behave
>>> differently so, ideally, we should solve the problem for good.
>>
>> Which *good* fix means renaming however its looked at. What about:
>> theMajor and theMinor ?
>>
>> AFAICT from theory they are only needed public by code which converts
>> to/from a string. Maybe not even that. If that can be avoided major_
and
>> minor_ private members might be available.
>>
>> BTW: the polish update should include my earlier comments about the
>> agnostic naming and SourceLayout position of the class itself.
>
> By "include comments", do you mean implement them or add a source code
> // TODO comment?
>

I mean implement. It's polish after all.

Amos
Received on Wed Aug 25 2010 - 03:07:05 MDT