Re: [PATCH] improve hack in src/base/TextException.h

On 02/22/2011 02:22 PM, Amos Jeffries wrote:
> On Tue, 22 Feb 2011 10:06:59 -0700, Alex Rousskov wrote:
>> On 02/22/2011 08:36 AM, Tsantilas Christos wrote:
>>> On 02/22/2011 04:47 PM, Kinkie wrote:
>>>> because it involves tricks to embed the same code again and again in
>>>> different compilation units to perform the caching.
>>
>>> Well, I do not have strong opinion on that, we can remove caching if
>>> we do not like. But this is a problem which it is solved and I doubt
>>> that it can cause more problems. If we believe that every time we are
>>> going to add a new module or feature we have to spend time for a new
>>> solution/hack to fix it, then we should remove it. This is just my
>>> opinion.
>>
>> I may have been the one who suggested this optimization, but I am not
>> strongly attached to it, and I am not against removing it if it causes
>> more problems than it solves.
>>
>> I do not think adding a few bytes to every .o file to store the computed
>> hash value is a problem, but others may find bigger flaws with the
>> approach, of course.
>>
>> Perhaps others can vote and break the current tie? Current code caches
>> the source code file name hash to avoid recomputing it for every failed
>> Must(). Must()s do not fail often, but, unlike assert()s, they may fail
>> when there are I/O and compatibility failures. The cost of such caching
>> is a few extra bytes added to every .o file that uses TextException. The
>> cost of not caching is computing a simple hash for a 10-40 byte string
>> (depending on the source file and the compiler).
>
>
> FWIW: The argument that pushed me from "dont care" to "may as well do
> it" last time. Was that on high performance systems (only) the caching
> method is faster and the new code using Must() was being relied on in
> areas like adaptation so it *would* cause them performance hits.
>
> AFAICT the rest of the code is explicit in its failure handling so this
> does not matter there in the slightest.
>
> IMO, its three hacks fighting it out. None of them good, but our goal it
> to get certain compilers to STFU without letting it produce bad code.
> So far __attribute__ appears to be the smallest since it does not add
> extra bytes on top of the calculation function OR extra work to the .o.

The class-based solution does not add extra bytes either. Not sure what
you meant by extra work, but the class-based solution does not add any
runtime overhead.

> The non-GCC appear not to care in the slightest, which is telling since
> they are so much more picky with code style.
>
>>
>>>> IMVHO it is not
>>>> even worth the time we have collectively spent in this discussion ;)
>
> Amen, Hallelujah, and anyone got a big enough rug to sweep this under? ;P
>
>>
>> That is probably true, at least short-term. FWIW, I would not bother
>> keeping this pandora box open if it were not for the GCC-specific
>> __attribute__ that was added by those who opened it :-).
>
> Answer me this: GCC specific complaint, why not a GCC specific fix?

The complaint is most likely not specific to GCC (i.e., there are
probably other compilers that do or will produce the same "unused
static" warning). The __attribute__ fix is definitely specific to GCC.

Alex.
Received on Sun Feb 27 2011 - 22:27:21 MST