kicad-developers team mailing list archive

[Dick Hollenbeck <dick@xxxxxxxxxxx>]

Very nice work.  What you have for saving and loading seems future proof
now, i.e. "context free".

Just some comments below, but I think you are already at 99% perfection.



> === modified file 'CMakeLists.txt'
> -add_subdirectory(gerbview)
>  add_subdirectory(kicad)
>  add_subdirectory(pcbnew)
> +add_subdirectory(gerbview)
Above: Don't like alphabetical?


> {
>          Line = aReader->Line();
> +
> +        if( strnicmp( Line, "PcbPlotParams", 13 ) == 0 )
> +        {
> +            PCB_PLOT_PARAMS_PARSER parser( &Line[13], aReader->GetSource() );
> +
> +            try
> +            {
> +                g_PcbPlotOptions.Parse( &parser );
> +            }
> +            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()


> +            }
> +
> +            continue;
> +        }
> +
>
> === added file 'pcbnew/pcb_plot_params.cpp'
> --- pcbnew/pcb_plot_params.cpp	1970-01-01 00:00:00 +0000
> +++ pcbnew/pcb_plot_params.cpp	2011-01-27 14:23:38 +0000
> @@ -0,0 +1,383 @@
> +
> +/*
> + * This program source code file is part of KICAD, a free EDA CAD application.
> + *
> + * Copyright (C) 1992-2011 Kicad Developers, see change_log.txt for contributors.

If this is your code, put your name here if you want.

> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, you may find one here:
> + * http://www.gnu.org/licenses/old-licenses/gpl-2.0.html
> + * or you may search the http://www.gnu.org website for the version 2 license,
> + * or you may write to the Free Software Foundation, Inc.,
> + * 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA
> + */
> +

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 )
> +{
> +    return PCB_PLOT_PARAMS_LEXER::TokenName( aTok );
> +}
> +
> +
> +
> +void PCB_PLOT_PARAMS::Format( OUTPUTFORMATTER* aFormatter,
> +                              int aNestLevel ) const throw( IO_ERROR )
> +{
> +    const char* falseStr = GetTokenName( T_false );
> +    const char* trueStr = GetTokenName( T_true );

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(),


not the way you are doing it.  The lexer will not like what you are doing in
all cases, that is the reason for Quoted().  This pertains to
outputDirectory below, but it is a general statement.



> +    aFormatter->Print( aNestLevel+1, "(%s \"%s\")\n", GetTokenName( T_outputdirectory ),
> +                       CONV_TO_UTF8( outputDirectory ) );
> +    aFormatter->Print( 0, ")\n" );
> +}
> +

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

> +
> +
> +PCB_PLOT_PARAMS& PCB_PLOT_PARAMS::operator=( const PCB_PLOT_PARAMS &aPcbPlotParams )
> +{
> +    layerSelection = aPcbPlotParams.layerSelection;
> +    useGerberExtensions = aPcbPlotParams.useGerberExtensions;
> +    m_ExcludeEdgeLayer = aPcbPlotParams.m_ExcludeEdgeLayer;
> +    m_PlotLineWidth = aPcbPlotParams.m_PlotLineWidth;
> +    m_PlotFrameRef = aPcbPlotParams.m_PlotFrameRef;
> +    m_PlotViaOnMaskLayer = aPcbPlotParams.m_PlotViaOnMaskLayer;
> +    m_PlotMode = aPcbPlotParams.m_PlotMode;
> +    useAuxOrigin = aPcbPlotParams.useAuxOrigin;
> +    m_HPGLPenNum = aPcbPlotParams.m_HPGLPenNum;
> +    m_HPGLPenSpeed = aPcbPlotParams.m_HPGLPenSpeed;
> +    m_HPGLPenDiam = aPcbPlotParams.m_HPGLPenDiam;
> +    m_HPGLPenOvr = aPcbPlotParams.m_HPGLPenOvr;
> +    m_PlotPSColorOpt = aPcbPlotParams.m_PlotPSColorOpt;
> +    m_PlotPSNegative = aPcbPlotParams.m_PlotPSNegative;
> +    m_PlotReference = aPcbPlotParams.m_PlotReference;
> +    m_PlotValue = aPcbPlotParams.m_PlotValue;
> +    m_PlotTextOther = aPcbPlotParams.m_PlotTextOther;
> +    m_PlotInvisibleTexts = aPcbPlotParams.m_PlotInvisibleTexts;
> +    m_PlotPadsOnSilkLayer = aPcbPlotParams.m_PlotPadsOnSilkLayer;
> +    subtractMaskFromSilk = aPcbPlotParams.subtractMaskFromSilk;
> +    m_PlotFormat = aPcbPlotParams.m_PlotFormat;
> +    m_PlotMirror = aPcbPlotParams.m_PlotMirror;
> +    m_DrillShapeOpt = aPcbPlotParams.m_DrillShapeOpt;
> +    scaleSelection = aPcbPlotParams.scaleSelection;
> +    outputDirectory = aPcbPlotParams.outputDirectory;
> +    return *this;
> +}
> +
>

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
> +{
> +    if( layerSelection != aPcbPlotParams.layerSelection )
> +        return false;
> +    if( useGerberExtensions != aPcbPlotParams.useGerberExtensions )
> +        return false;
> +    if( m_ExcludeEdgeLayer != aPcbPlotParams.m_ExcludeEdgeLayer )
> +        return false;
> +    if( m_PlotLineWidth != aPcbPlotParams.m_PlotLineWidth )
> +        return false;
> +    if( m_PlotFrameRef != aPcbPlotParams.m_PlotFrameRef )
> +        return false;
> +    if( m_PlotViaOnMaskLayer != aPcbPlotParams.m_PlotViaOnMaskLayer )
> +        return false;
> +    if( m_PlotMode != aPcbPlotParams.m_PlotMode )
> +        return false;
> +    if( useAuxOrigin != aPcbPlotParams.useAuxOrigin )
> +        return false;
> +    if( m_HPGLPenNum != aPcbPlotParams.m_HPGLPenNum )
> +        return false;
> +    if( m_HPGLPenSpeed != aPcbPlotParams.m_HPGLPenSpeed )
> +        return false;
> +    if( m_HPGLPenDiam != aPcbPlotParams.m_HPGLPenDiam )
> +        return false;
> +    if( m_HPGLPenOvr != aPcbPlotParams.m_HPGLPenOvr )
> +        return false;
> +    if( m_PlotPSColorOpt != aPcbPlotParams.m_PlotPSColorOpt )
> +        return false;
> +    if( m_PlotPSNegative != aPcbPlotParams.m_PlotPSNegative )
> +        return false;
> +    if( m_PlotReference != aPcbPlotParams.m_PlotReference )
> +        return false;
> +    if( m_PlotValue != aPcbPlotParams.m_PlotValue )
> +        return false;
> +    if( m_PlotTextOther != aPcbPlotParams.m_PlotTextOther )
> +        return false;
> +    if( m_PlotInvisibleTexts != aPcbPlotParams.m_PlotInvisibleTexts )
> +        return false;
> +    if( m_PlotPadsOnSilkLayer != aPcbPlotParams.m_PlotPadsOnSilkLayer )
> +        return false;
> +    if( subtractMaskFromSilk != aPcbPlotParams.subtractMaskFromSilk )
> +        return false;
> +    if( m_PlotFormat != aPcbPlotParams.m_PlotFormat )
> +        return false;
> +    if( m_PlotMirror != aPcbPlotParams.m_PlotMirror )
> +        return false;
> +    if( m_DrillShapeOpt != aPcbPlotParams.m_DrillShapeOpt )
> +        return false;
> +    if( scaleSelection != aPcbPlotParams.scaleSelection )
> +        return false;
> +    if( !outputDirectory.IsSameAs( aPcbPlotParams.outputDirectory ) )
> +        return false;
> +    return true;
> +}
> +
>
>
>
> +void PCB_PLOT_PARAMS_PARSER::Parse( PCB_PLOT_PARAMS* aPcbPlotParams ) throw( IO_ERROR, PARSE_ERROR )
> +{

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