Re: [PATCH] Optimize HttpVersion comparison

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?

Cheers,

Alex.
Received on Wed Aug 25 2010 - 02:45:58 MDT