Re: [Gnash-dev] Sun Studio and general C++ correctness patches, plus GNASHRC duplicate filtering and plugin fixes

[Albert Lee]

On Fri, 11 Dec 2009 08:46:21 +0100, Benjamin Wolsey
<address@hidden>
wrote:
>> The inflated symbol table will have much more trouble with cache, which
>> will penalise runtime performance if you're using lazy symbol
resolution,
>> which is compounded by most of the log_* messages incurring a lookup
>> since
>> they use different symbols.
> 
> In theory, but again in practice we haven't had any indication that it
> is a problem. I count 700 log_* symbols, by the way.
> 
Last time I built it it had 1022 log_debug, and something like ~2000 total
for me.

> The more important argument is that there are important differences
> between boost::format and printf:
> 
> 1. boost::format automatically makes use of an available ostream
> operator<< for a class, and we use this frequently. It's not possible,
> and certainly not legal, to send these to vsnprintf.
> 
> 2. boost::format has other substitutions, such as %1% and %2%, which are
> used in some places and not recognized by printf.
> 
> 3. The printf substitutions are no longer used correctly in many places
> (for instance we have %s for some numeric values) because it is no
> longer checked, and boost::format doesn't care.
> 
> 4. Even when they are used correctly, printf always made problems with
> pointer sizes, because they require a different substitution on
> different platforms. The abomination that was used to work around this
> was dropped when it was no longer needed.
> 
> In short, using va_args and printf will at best give nonsense, and at
> worst format your hard drive and start a nuclear war.
> 

Ouch. Maybe the right thing to do here is to edit all the log_* calls use
boost::format syntax so the templated conversion is unnecessary... I
suppose this can be done programmatically.

>> http://stdcxx.apache.org/doc/stdlibug/13-5.html describes the
std::remove
>> algorithm, which bubbles the all non-removed elements to the beginning
of
>> the sequence so should be logically equivalent to having relocated the
>> removed elements to the end.
> 
> One doesn't follow from the other... 
> 
> vector<int> v = { 1, 2, 3, 4, 5 };
> remove(v.begin(), v.end(), 3));
> 
> The result at least in the gcc stdlib implementation is
> 
> 1, 2, 4, 5, 5.
> 
> The standard specifies only that the container remains the same size,
> the non-removed elements are copied to the front, and the function
> returns an iterator pointing to the end of the non-removed sequence.
> There are no guarantees about what is or isn't at the end (though
> generally it will be the original values, because otherwise the
> algorithm would do unnecessary copying).
> 

Ah... I was operating on a mistaken assumption about the insertion
behaviour. Thanks for the clarification.

>> This is O(n^2) and the commented code should be O(n*log n) if I used a
>> list instead of the typedef'd vector. I don't see any point for a short
>> string so I'll just remove the commented code.
> 
> We're talking about a container of about 4 strings. A lunatic might
> possibly multiply that by a factor of 10. Why would you need to parse 40
> gnashrcs? I don't know. He's a lunatic. In both cases, the number of
> insertions ought to be relatively low. So we should have code that is
> simple, self-documenting, and easy to read before even thinking about
> complexity.
> 

OK, replaced the loop body with: l.remove(*i); l.push_back(*i);

>> I don't even know what NellyMoser is. ;)
>> So are you just suggesting substituting magic constant for the #define
>> (that file seems to be full of them)?
> 
> Yes, I'm suggesting using a named const double as a magic constant. Also
> I'm suggesting removing the file, but I'm not sure who knows enough
> about it to make that decision.
> 
> I know ffmpeg handles NellyMoser (it's an audio codec) if the version is
> new enough and that file is a fallback for older versions. But I don't
> think it works, so the advantage over having no fallback isn't very
> obvious.
> 

OK, made the change as suggested and I'll leave the removal decision to
someone who can bother testing the current code.


> bwy
> 

Thanks for the info!

-Albert