kicad-developers team mailing list archive

[Wayne Stambaugh <stambaughw@xxxxxxxxx>]

Brian,

On 3/13/19 6:32 PM, Brian wrote:
> Wayne,
> 
>> What exactly are you serializing to and from?  If it's just a string
>> saved to a loaded from a database, why not just add public methods to
>> the plugin object to serialize to and from a string?
> My goal is to have my database contain legacy-format symbol data (since
> the new format doesn't exist in code yet -- my goal is not to roll my
> own format or implement a different one, but only to divert what would
> be in a file into a database instead).  I chose to encapsulate the way I
> did because:
> 
> 1. SCH_LEGACY_PLUGIN_CACHE has no public interface in any header file;
> it is declared and defined solely within sch_legacy_plugin.cpp.  (There
> is only a forward declaration in sch_legacy_plugin.h)  To do as you
> suggest, I would have to write an entire, correct declaration for
> SCH_LEGACY_PLUGIN_CACHE that my plugin code could see.  That seems like
> a lot of extra work just to share a small subset of the functionality.

You only write what you need for each plugin.  We have plugins that only
load symbol libraries and will complain if you try to write the library
or read/write the schematic.  The library caching technique is
completely up to the implementer.

> 2. My plugin doesn't need any other part of SCH_LEGACY_PLUGIN_CACHE or
> SCH_LEGACY_PLUGIN, as it only handles symbols and not entire schematics
> and has its own caching scheme.

The reason I wrote it this way is because I knew that the current file
format was going to be short lived and I didn't want to expose it any
more than I had to.  That being said, I still don't know exactly what
your are parsing and formatting.  Is it the full symbol library file
format definition or just LIB_PART definition?  If it's just LIB_PART
(which I suspect it is), then there is no need to break out all of the
symbol library file parser code.  Only loadPart(), savePart(), and its
dependencies need to be broken out.

> 3. All of the actual serialization code could be static, but is defined
> as member methods on SCH_LEGACY_PLUGIN_CACHE (the only actual member
> variable dependency is version number, which is easily passed as an
> argument).  I don't want my plugin to have to instantiate an entire
> SCH_LEGACY_PLUGIN_CACHE just to turn a LIB_PART to/from a string.  Ergo,
> LEGACY_PART_READER / LEGACY_PART_WRITER are entirely static.

You could make them static in the SCH_LEGACY_PLUGIN_CACHE as well and
access them through the SCH_LEGACY_PLUGIN as static.

> 4. I could have just copied the entirety of the to/from string code from
> SCH_LEGACY_PLUGIN_CACHE into my plugin's code, but that seemed like the
> wrong thing to do for several reasons; code reuse being the primary.

This code is almost identical to the legacy footprint library code.
It's on my todo list to factor out the common code from these two
objects and create a reusable base object.

> 
>> That's how we
>> handled this in the board plugin.  I would also prefer to leave the
>> parsing and formatting code in the current file.  The formatter part of
>> this is going to go away most likely during v7 development but possibly
>> during v6 development depending upon how fast I can get the new file
>> format implemented.
> A rebuttal:
> 
> I believe this is a way in which our underlying coding philosophies
> differ.  I am a fan of tight encapsulation and small-as-practical .cpp
> files.  While I will occasionally embed trivially-small helper classes
> in the headers/implementations of other classes, I generally tend to
> take a header-and-implementation-per-class approach.  The entirety of
> EESCHEMA could live in a single .cpp file, or every function definition
> could be in a separate file.  Beyond the requirements of the language,
> how code is divided into files is a religious argument.  I'm happy to
> defer to your seniority, but I wanted to explain the reason I made the
> choices I made.

Your coding philosophy is not much different than mine.  I just had the
advantage of knowing that half the code in sch_legacy_plugin.cpp/h was
going away and that I didn't want anybody messing with it.  That's why
it lives in a single file.  The board file format parser and formatter
live in separate files precisely for this reason.

> 
> As an aside, having my modifications removing large swaths of
> planned-obsolete code from sch_legacy_plugin.cpp seems like it would
> make your job easier when it comes time to eradicate it from the plugin;
> I've already sifted through and yanked out all of the helper methods.

I agree but once the legacy plugin is truly legacy, it will be frozen
except for bug fixes.  I only have to keep an eye on one file to make
sure no one makes any unapproved changes to it.  As project lead, that
make my life easier.

> 
> As a second aside, having the read/write code for the new s-expression
> format similarly encapsulated would be awfully nice, so that I can
> change my ODBC plugin to use it when it's published, without having to
> go on another refactoring mission, and maintain all the same points as
> mentioned above.

When the new symbol library and schematic file format plugin is written,
the parser and formatter will be split out for general purpose use.

> 
>> Also, please do not force the GCC ABI version in the CMake config.  You
>> should be passing -DCMAKE_C_FLAGS="-fabi-version=2" and/or
>> -DCMAKE_CXX_FLAGS="-fabi-version=2" on the command line when you call
>> CMake to generate the build files if you need a specific GCC ABI version
>> or any other personal compiler flags.  This kind of thing can cause all
>> kind of headaches for developers so it's frowned upon by most projects.
> Understood.  On that, I have no argument!
> 
> Kind regards,
> -Brian
>>
>> Cheers,
>>
>> Wayne
>>
>> On 3/13/2019 12:16 PM, Brian wrote:
> <snip>
> 
> _______________________________________________
> 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