Re: [PATCH] Optimize HttpVersion comparison

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.

Amos
Received on Wed Aug 25 2010 - 02:28:06 MDT