← Back to team overview

kicad-developers team mailing list archive

Re: Patch: Extract LIB_PART read/write from SCH_LEGACY_PLUGIN_CACHE



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. 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. 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. 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.

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.

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.

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.

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,



On 3/13/2019 12:16 PM, Brian wrote:

Follow ups