kicad-developers team mailing list archive

[Wayne Stambaugh <stambaughw@xxxxxxxxx>]

On 10/06/2018 03:48 PM, John Beard wrote:
> Hi Wayne,
> 
> On Sat, Oct 6, 2018 at 5:29 PM Wayne Stambaugh <stambaughw@xxxxxxxxx> wrote:
>> No problem.  I will test your patch once you post it.
> 
> Patch attached. Indeed, it was a missing include in the header that
> probably worked until now due to serendipitous include ordering in the
> existing CPPs.
> 
> This actually illustrates one nice thing about having unit tests CPPs
> that include only a single KiCad header - they make sure that header
> and any transitive includes are compilable even when the test CPP only
> includes that one header (i.e. as recommended by the code guidelines:
> 6.2 Headers Without Unsatisfied Dependencies [1]).

This is why there is a section for this in the coding guidelines.  Some
of the issues have been around for a long time but there are many others
that have been introduced with new code.  We really need to do a better
job at this.  We really shouldn't have to right unit tests to ferret out
all of the invalid headers.

> 
> Another way this can improved is when you have a .cpp/.h pair for a
> self-contained class, the .cpp should include the corresponding .h
> first before any other include. This ensures the header is compilable
> by itself, and has no implicit inclusion criteria for use. This is
> sometimes, but not always true in KiCad. (In this case, there is no
> .cpp anyway, so the unit test was the first CPP to attempt to include
> by itself).
> 
>> The problem is all of the inline functions defined in convert_to_biu.h.
>> Until they are replaced with per application equivalents, we will
>> continue to have to compile every common source file multiple times that
>> use anything in this file.
> 
> Right, but in the case of dialog_page_settings.cpp in particular, the
> only definition actually needed from here was IU_PER_MILS,
> which can be equivalently retrieved, via the virtual BASE_SCREEN
> interface, from the derived class, where IU_PER_MILS is properly set
> for each compiled target (eeschema, pl_editor, pcbnew).
> 
>> This is odd as they are both in the COMMON_SRCS list.  Actually,
>> color.cpp is included twice.  Once in KICAD_SRCS and once in COMMON_SRCS
>> which includes KICAD_SRCS.  We should remove color.cpp from either
>> KICAD_SRCS or COMMON_SRCS.  I see no need to build it twice.
> 
> Even more odd. Removing one of the colors.cpp from the
> common/CMakeLists.cpp didn't stop qa_common having to specifiy it
> again, though.
> 
> For colors.cpp specifically, this is an issue which will be resolved
> when, as promised in colors.h, the removal of legacy support will make
> the global colour table obsolete. So it's perhaps more of a curiosity
> than an issue?
> 
> Regardless, if anyone can shed light on this, I'd appreciate it, as it
> probably indicates something fishy somewhere?
> 
> Cheers,
> 
> John
> 
> [1]: http://docs.kicad-pcb.org/doxygen/md_Documentation_development_coding-style-policy.html#header_depends
>