kicad-developers team mailing list archive

[Marco Mattila <marcom99@xxxxxxxxx>]

On Fri, Jan 28, 2011 at 12:21 AM, Dick Hollenbeck <dick@xxxxxxxxxxx> wrote:
>> === modified file 'CMakeLists.txt'
>> -add_subdirectory(gerbview)
>>  add_subdirectory(kicad)
>>  add_subdirectory(pcbnew)
>> +add_subdirectory(gerbview)
> Above: Don't like alphabetical?

I love alphabetical. However, since files produced by make_lexer in
pcbnew were needed when compiling gerbview and since I was initially
not able to get make_lexer work in gerbview CMakeLists.txt, I just
compiled pcbnew first. Now, after testing this a bit more, it seems
that my problem with make_lexer in the gerbview CMakeLists.txt was
using ../pcbnew/... as arguments to make_lexer instead of
${PROJECT_SOURCE_DIR}/pcbnew... And with further testing it turns out
that it is not necessary to include pcbplot.h in
printout_controler.cpp at all and, thus, it is not necessary to change
the gerbview CMakeLists.txt in any way.

>> +            catch( IO_ERROR& e )
>> +            {
>> +                D( printf( "pcbplotparams parsing error: '%s'\n",
>> +                           CONV_TO_UTF8( e.errorText ) ); );
>
> We can probably do better than this, using wxMessageWhatever()

I'll add a dialog here.

> Following function is bound to conflict in global namespace, make static and
> shorten name considerably is suggested.  If its static and local, you could
> make it EXTREMELY short even a just a couple of characters.
>
>> +
>> +const char* GetTokenName( T aTok )

Will be changed to static. Could maybe use namespace, too (?). But why
very short? GTN isn't very informative.

> Any "complex" string that is capable of having either whitespace or quotes
> in it should be wrapped using
> aFormatter->Quoted( CONV_TO_UTF8(blah) ).c_str(),

Will be fixed.

> Is this operator=() different than what the compiler will automatically
> generate for you?

As there are no pointers as class members, writing the operator code
isn't strictly necessary. The current code is not assigning all
members, since some of them are always calculated before plotting. But
the automatic version should work fine, too.

> This is a lot of code below, should be present only if used I guess:
>
>> +bool PCB_PLOT_PARAMS::operator==( const PCB_PLOT_PARAMS &aPcbPlotParams ) const

It is used to check if any settings were changed in the plot dialog.
OnModify() is called only when changes were made.

> This here is better done as shown below:
>> +    T token;
>> +    while( ( token = NextTok() ) != T_RIGHT && token != T_EOF )
>> +    {
>
>    T token;
>    while( ( token = NextTok() ) != T_RIGHT )
>    {
>        if( token == T_EOF )
>                Unexpected( T_EOF );

Will be fixed.

marco