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

[Albert Lee]

Hi,

On Thu, 10 Dec 2009 21:14:38 +0100, Benjamin Wolsey
<address@hidden>
wrote:
> Am Donnerstag, den 10.12.2009, 13:30 -0500 schrieb Albert Lee:
>> * gnash-01-sunpro.diff:
>> Fix compatibility with Sun Studio. Major changes:
>> - Use boost::assign::list_of instead of map_list_of to initialise
default
>> tag loader list, which is now a vector since it's going to be inserted
>> into
>> a map anyway. Studio doesn't match the type of the function pointer
when
>> map_list_of is used for some reason.
> 
> Seems okay.

A potential enhancement to make it worth using a map here is if
TagLoadersTable could gain an interface to insert the entire map directly
instead of adding each pair individually.

> 
>> - Reimplement log_{debug,error,...} in a non-templated version using
>> Boost
>> macros as the templated one has issues when called from the GUI, and
>> generates 1000+ symbols for log_debug variants, which is ridiculous.
You
>> lose the strong typing of boost::format so only enable for Studio right
>> now.
> 
> What issues?

The compiler doesn't see a prototype for log_debug when compiling
gui/gui.cpp. The other consumers of the functions seem to be fine with it.
I also tried gnash::log_debug explicitly to the same effect.

> 
> I don't think it's a good idea to have two implementations of this in
> trunk, let alone one that uses varargs. It's quite possible to have it
> as an OS-specific patch if the compiler isn't capable of handling it. 
> 
> The large number of symbols is admittedly not very nice, but in practice
> (assuming it compiles), where is the problem?
> 

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.

>> - #include <clocale> instead of <locale> for setlocale(3) and LC_ALL.
> 
> Fine.
> 
>> * gnash-03-gnashrc.diff:
>> Filter duplicates from GNASHRC string.
> 
> That patch needs cleaning up; code should be removed, not commented out.
> Also, std::remove doesn't copy anything to the back of the sequence at
> all, so it needs improving logically too.

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. The removed elements are retained, hence the
need for std::erase in normal usage. There's only one removed value here at
most.

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.

> 
>> * gnash-04-plugin.diff:
>> - Use strcasecmp(3) conditionally and include strings.h directly for it
-
>> cstrings does not exist.
> 
> Use StringNoCaseEqual from StringPredicates.h here in both cases;
> std::string::compare is hardly an replacement for strcasecmp either. And
> then drop the strings.h include.
> 

Didn't know that existed. Does the strncpy look okay to you?

>> - Use std namespace consistently for standard C functions.
> 
> This is good.
> 
> You also defined M_SQRT_1_2 on platforms where it isn't defined. I don't
> see any need to use this non-standard value at all. It can easily be
> replaced by a local (non-static!) const double with exactly the same
> effect. I'm not even sure the NellyMoser code is used, or works if it is
> used.

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)?

I'll resubmit the plugin and gnashrc patches. I'm more comfortable as a C
programmer (in case it's not obvious) so please mention if anything else
looks strange to you.

Thanks for the feedback,

-Albert