Code review comment for lp:~cern-kicad/kicad/bugfix_1275319

On 02/06/2014 04:26 PM, Maciej Sumiński wrote:

> It is not my intention to increase the number of bits in m_VisibleElements - I prefer
> to leave it as it is now, particularly that it is saved as a 32-bit number in
> .kicad_pcb files and I would like to preserve compatibility (so if older versions
> expect 32-bit number there

Note, that if we were always bound to allow older versions of KiCad to load newer files,
then it would be impossible to add new features which needed saving on disk.

I value new features far more. We can all stop writing code and wasting time on a mailing
list if we are not to write features requiring file format changes. It would essentially
mean that we could only make improvements which do not require change.

The pay is too low already. If you can find a way to make improvements without also
making changes, then I am in the wrong universe.

> - let them have it and do not crash/do weird things on load
> of PCBs holding 64 bits for elements visiblity).

Not really an issue, if you parse an ASCII 64 bit number into a 32 bit binary integer, in
old code, the lower 32 bits are likely preserved. So in this case you might have been OK.
 If not, folks can stick with their old software and old BOARDs.

> Saving netnames layers visibility is
> not really crucial, it can be restored basing on visibility settings of appropriate
> copper layers.

I like the feature! And I like the 33rd feature which I want to save to disk, whatever it is.

I would like to save it to disk, and have it be BOARD centric, not project centric.

Eventually we'll hit some kind of wall, if not here, now, then sometime.
Saving a bit map as an ASCII integer is only one possibility anyways. There are numerous
alternatives for expansion:

a) save a 64 bit integer in ASCII.

b) use s-expressions to name each bit.

c) An arbitrartily long ASCII binary number
0000_1011_1110_0011....

This can extend far off the page if needed.

Parsing that would be pretty easy, the underscore saves a human some counting. At some
point you quietly switch to a bitmap of arbitrary length. But since you've hidden the
implementation from the client code, that is nobody's damn business.

d) blah blah blah

> The change from int to long resulted only from the fact that in the back of my head I
> remember that int is guaranteed to be 16 bits,

Fully incomprehensible. I don't know where 16 bits even enters into it. Find a computer
that uses 16 bits and you've found one that should not be running KiCad.

The lesson here is that "long" is

a) not necessarily longer than int, and

b) not known until you pick a machine and a compiler. So you don't get 64 bitness if
thats what you need.

> so maybe on some platforms the code
> would not run as expected. But as it works on the major platforms then I simply do not
> insist on changing that.

int64_t is 64 bits, guaranteed.

Anyway - the patch fixes the problem even when
> m_VisibleElements stays as integer, so what do you think about the rest of the
> changes?

I have not looked at it, I can if nobody else want's to. The original patch came to me
via email.

> Regards, Orson