← Back to team overview

kicad-developers team mailing list archive

Re: [PATCH] Ortographic mode for 3D viewer and minor cleanup


Hash: SHA1

On 10/07/2010 08:53 AM, Dick Hollenbeck wrote:
> Alex,
> congrats on your patch getting accepted and applied!
> I have just a few comments below that will make your next patch even better.
> Thanks,
> Dick


Thanks for the confidence. :)
I'm looking forward to making more contributions in the future,
hopefully, of more value. Although my narrow experience makes tasks such
as working on the new library structure seem daunting, I'm constantly
looking for something I can improve.

I would like to explain why I made some modifications that are more or
less debatable. I am not trying trying to prove that I am right, just to
share some thoughts.

I compile with -pedantic. I made this habit while working on a
cross-platform HPC project. -pedantic made sure that the code would work
with other compilers (including MSVC++), but also caught a lot of small
mistakes that could cause the program to segfault. I find it a very
useful tool. Although a very anal one, I find its benefits greater than
the sum of its deficiencies.

When I first built kicad with -pedantic, the number of warnings I got
made it impossible to catch the meaningful ones. The extra "," at end of
enumerator list was one of the more common ones. While the lack of ","
is noise for you, its presence is noise for me. I believed that removing
it would not cause any stir.

> Looks like you had one to many spaces on these lines, should have been 4.
Sorry about that. Careless mistake on my part.

> Spelling error in ortographic
Careless mistake again.

> Please put a blank line above comments if comments are only thing on
> that line and they are not a comment continuation.
> needed a blank line here, above the single line comment.
Of course.

> No need for this edit, last enum can have a trailing comma just fine.  I
> write code like that.
> ditto, leave the comma in, this edit is noise.
> More noise, and more noise many times below that I do not mention again.
> Noise.
- -pedantic made me do it. I'm innocent :P

> The space between \n E is not needed, removing the \ before the E was a
> good edit.
- -pedantic pointed me to this :)

> selection spelling
>> +    int           m_Selected_Tool;              // Pour editions: Tool (Dcode) selectionn�
Didn't catch that. I had some files that KDevelop was complaining about
the encoding, and this was one of them. I failed to see why the
complaint occurred, and as a result, failed to see the horrible monster
I had created. I apologize about this.

> comment start is off by one:
>> +    FILLED_WITH_BG_BODYCOLOR    /* Poly, Square, Circle, Arc = option Fill
>>                                    * with background body color, translucent
>>                                    * (texts inside this shape can be seen)
>>                                    * not filled in B&W mode when plotting or
Careless mistake.

> These were my printf()s and I can assure you that nobody else needs them
> except me.  I like printf() as a better IO model than std::out <<
Some pedantic warnings as well led me here. I thought that std::cout
might be better because of stronger type-safety. If you use %p, can you
please cast the pointer to (void *) ? It fixes the -pedantic warning.

> So please don't do this in the future.  

I don't understand why printf() is preferred over std::cout in C++ code,
but I will comply.

Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/


Follow ups