kicad-developers team mailing list archive

[Wayne Stambaugh <stambaughw@xxxxxxxxx>]

On 6/27/2016 6:20 PM, Simon Richter wrote:
> On 27.06.2016 23:47, Michael Steinberg wrote:
> 
>> === modified file 'bitmap2component/bitmap2cmp_gui.cpp'
>> --- bitmap2component/bitmap2cmp_gui.cpp	2016-06-08 06:32:01 +0000
>> +++ bitmap2component/bitmap2cmp_gui.cpp	2016-06-27 19:02:56 +0000
> [...]
>> +    // TODO: Should the config really not be written if BM2CMP is
> iconized?
> 
> Good question.

Indeed.  I'm guessing saving the iconized config state would confuse the
user when they launch bitmap2cmp and it opens in the iconized state.
I'm not sure it would do this but I wouldn't bet against it.

> 
> Meta-question: Should we use /// @todo for these to get doxygen to
> compile a list?

Yes.  Use doxygen syntax for todo comments.  It makes a nice todo list
in the dev docs rather than having to grep the source for todo.

> 
>> -    delete m_config;
>> +    m_config->Flush();
>> +    m_config.reset();
> 
> In the long run, we'd need to have error handling code here. As it is
> now, the patch is certainly an improvement over the old state, where a
> destructor was tasked with flushing the config to disk, and had no
> recourse on error.

wxFileConfig::Flush() calls wxLogError if it cannot write the config
file so the user will at least get a warning but there is no recourse if
the file cannot be written.  I'm not sure what said recourse would be.
The kicad config files have to be saved in the users config path so if
the config file cannot be written, the user is probably going to have
bigger issues to worry about than a kicad config file.  I don't know if
giving the user a chance to write the config file to a different folder
is useful.

> 
> [...]
> 
>> -    cfg = new wxFileConfig( wxT( "" ), wxT( "" ),
> configname.GetFullPath() );
>> -    return cfg;
>> +    return std::unique_ptr<wxConfigBase>(
>> +        new wxFileConfig(wxT(""), wxT(""), configname.GetFullPath())
>> +    );
> 
> C++14 has std::make_unique, but we'll cross that bridge when we reach it.
> 
>> -        wxConfigBase* config = GetNewConfig( fn.GetFullPath() );
>> +        auto config = GetNewConfig( fn.GetFullPath() );
>>          config->Write( HOTKEYS_CONFIG_KEY, msg );
>> -        delete config;
> 
> This has subtle potential for errors -- while I'm normally in favour of
> using "auto", spelling it out would help here in case the function
> signature is changed back.

Potential for errors aside, I think auto makes the code less readable
and yes code readability matters.  In this case, I have to go look up
the return value of GetNewConfig() because I don't know what an auto
type is.  I do know what a wxConfigBase* type is.  I'll just warn
everyone now that I am not a big fan of using certain syntax just for
the sake of being "C++ correct".  If it makes the code less readable, I
will resist committing it.

Cheers,

Wayne

> 
>    Simon
> 
> 
> 
> _______________________________________________
> Mailing list: https://launchpad.net/~kicad-developers
> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help   : https://help.launchpad.net/ListHelp
>