kicad-developers team mailing list archive

[Wayne Stambaugh <stambaughw@xxxxxxxxx>]

I don't know if it's still true but msvc didn't include stdint.h so
making this policy would have left msvc users without a solution.  This
may no longer be true and I seem to remember that someone was providing
an implementation of stdint.h for msvc.  If this is no longer the case,
then it could be added to the coding policy.

On 3/25/19 3:02 PM, Jon Evans wrote:
> Any reason not to just make a policy moving forward to define anything
> related to file formats using stdint types so that there are no
> architecture variations possible?
> 
> On Mon, Mar 25, 2019 at 2:59 PM Jeff Young <jeff@xxxxxxxxx
> <mailto:jeff@xxxxxxxxx>> wrote:
> 
>     Hi JP,
> 
>     I’m afraid I just move the decl (and its comment) from another
>     file.  It appears the author
>     was Henner Zeller in 3f57fa5d2443a085c9fa243c615d71dc470a0107.  
> 
>     Commit comment was: 
> 
>         Change time_t in the functions that deal with timestamps to a new 
>         typedef timestamp_t (defined as a long).
>         that makes sure the c++ side and swigged Python side agree on the 
>         type, because time_t create problems in Python scripts.
> 
>     Cheers,
>     Jeff.
> 
> 
>>     On 25 Mar 2019, at 15:51, jp charras <jp.charras@xxxxxxxxxx
>>     <mailto:jp.charras@xxxxxxxxxx>> wrote:
>>
>>     Le 25/03/2019 à 16:22, Wayne Stambaugh a écrit :
>>>     On 3/24/2019 2:34 PM, Seth Hillbrand wrote:
>>>>     Am 2019-03-24 13:23, schrieb Jon Evans:
>>>>>     Hi all,
>>>>>
>>>>>     Another question from this forum thread:
>>>>>     https://forum.kicad.info/t/before-giving-up-on-kicad-one-last-attempt-erc-misery/15933/67
>>>>>
>>>>>
>>>>>     timestamp_t is defined as "long", with a note that swig can't
>>>>>     handle
>>>>>     int32_t.
>>>>
>>>>     I don't understand this comment.  SWIG includes stdint.i which
>>>>     explicitly defines the exact integral types.  Maybe Jeff can
>>>>     shed some
>>>>     light here?
>>>>
>>>>
>>>>>     This means that timestamp_t will be 32-bit on many platforms, but
>>>>>     64-bit on Linux and MacOS running on 64-bit hardware.  Apparently
>>>>>     there is at least one bug here involving the Eagle importer writing
>>>>>     out library files with 64-bit timestamps, which 32-bit KiCad cannot
>>>>>     open.
>>>>
>>>>     This is a problem in ParseHex().  If it gets a hex value that
>>>>     overflows,
>>>>     it throws an error, stopping the processing.  So this isn't
>>>>     specific to
>>>>     the Eagle plugin but rather a 32-bit/64-bit issue.  We allow
>>>>     64-bit to
>>>>     be written to file but only generate a time_t (32-bit) value for
>>>>     new,
>>>>     internal stamps.
>>>>
>>>>     The Eagle processor creates its own "timestamp" by hash which is
>>>>     64 bits
>>>>     on a 64 bit machine.
>>>
>>>     Do you mean our Eagle plugins are not truncating 64 bit time
>>>     stamps in
>>>     Eagle files?  If so, the problem needs to be fixed here.  AFAIK,
>>>     KiCad
>>>     has always used 32 bit time stamps.
>>
>>     The issue is the fact the legacy plugin currently uses a long as time
>>     stamp, but do not truncate if to 32 bits when writing a .sch file.
>>     So because Eagle importer generate a long value, the legacy plugin
>>     writes 8 hexa chars on 32 bits platform, but can write more than 8
>>     chars
>>     on 64 bits platforms
>>
>>>
>>>>
>>>>>     Q1: Is there actually a bug here? maybe someone more familiar
>>>>>     with the
>>>>>     Eagle importer can take a look.
>>>>
>>>>     Yes.  Definitely bug.  We should be trimming the hash-based
>>>>     timestamp to
>>>>     32-bits.
>>>
>>>     You would also have to add checking for duplicate time stamps due
>>>     to the
>>>     truncation of the upper 32 bits.
>>>
>>>>
>>>>>     Q2: Shouldn't we change the type of timestamp_t to be either
>>>>>     "int" (if
>>>>>     we want it to be 32-bit) or "long long" (if we want it to be
>>>>>     64-bit)?
>>>>
>>>>     I think 32-bits is the correct way to go here and using uint32_t
>>>>     should
>>>>     work but I may be missing an important detail.
>>>>
>>>>>     My first thought is that we should change timestamp_t to be
>>>>>     32-bit on
>>>>>     all platforms, but I'm not sure the best way to handle existing
>>>>>     files
>>>>>     that have been written out with 64-bit timestamps.
>>>>
>>>>     This is problematic.  I think we could patch the reader to
>>>>     handle 64-bit
>>>>     values but store 32-bit.
>>>>
>>>>     -Seth
>>
>>     I have a fix for that (it forces 32 bits in timestamp_t and allows 64
>>     bits when reading a file).
>>
>>     Annotation should already take care of duplicate timestamps (and
>>     replace
>>     duplicates if any) unless bug.
>>
>>     The right fix is to use uint32_t for timestamps, at least for the
>>     current file format.
>>
>>     @Jeff, you added a comment in common.h about an issue with Swig.
>>     What is this issue you encountered when using uint32_t as typedef for
>>     timestamp_t with swig (I did not see an issue during my tests)?
>>
>>     -- 
>>     Jean-Pierre CHARRAS
> 
>     _______________________________________________
>     Mailing list: https://launchpad.net/~kicad-developers
>     Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>     <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
>     Unsubscribe : https://launchpad.net/~kicad-developers
>     More help   : https://help.launchpad.net/ListHelp
> 
> 
> _______________________________________________
> 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
>