kicad-developers team mailing list archive
-
kicad-developers team
-
Mailing list archive
-
Message #39900
Re: timestamp_t bit width
MSVC 2010 includes it.
https://stackoverflow.com/questions/126279/c99-stdint-h-header-and-ms-visual-studio
*Drew Van Zandt*
On Mon, Mar 25, 2019 at 5:25 PM Wayne Stambaugh <stambaughw@xxxxxxxxx>
wrote:
> 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
> >
>
> _______________________________________________
> 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
>
Follow ups
References