:(

> Date: Wed, 28 Apr 2010 16:06:07 +0200
> From: mstsxfx@gmail.com
> To: misutkajunior@hotmail.com
> CC: pdfedit-devel@lists.sourceforge.net
> Subject: Re: [Pdfedit-cvs] pdfedit/src/xpdf/xpdf SplashOutputDev.cc,1.6,1.7
>
> On Wed, Apr 28, 2010 at 01:11:29PM +0000, Jozef Misutka wrote:
> > > On Tue, Apr 27, 2010 at 11:19:32PM +0200, Michal Hocko wrote:
> > > > On Tue, Apr 27, 2010 at 08:07:35PM +0300, Jozef Misutka wrote:
> > > > > On Tue, 27 Apr 2010 15:52:35 +0300, Michal Hocko <mstsxfx@gmail.com> wrote:
> > > > >
> > > > > >On Tue, Apr 27, 2010 at 01:37:13PM +0300, Jozef Misutka wrote:
> > > > > >>On Mon, 26 Apr 2010 20:29:05 +0300, Michal Hocko
> > > > > >><mstsxfx@gmail.com> wrote:
> > > > > >>
> > > > > >>please michal, fix this issue as I asked you yesterday. thank you
> > > > > >>
> > > > > >>>>
> > > > > >>>>5>..\..\src\xpdf\xpdf\XRef.cc(815) : error C3861: 'atoll':
> > > > > >>>>5>..\..\src\xpdf\xpdf\XRef.cc(825) : error C3861: 'atoll':
> > > > > >
> > > > > ><muttering>
> > > > > >Well, you have ACKed the patch so I assumed you have at least looked at
> > > > > >it or tried to compile it. Now you are asking me to fix it even though
> > > > > >it doesn't work only on Windows (the function is POSIX).
> > > > > ></muttering>
> > > > >
> > > > > Does anyone in your company recompile everything you commit? i would
> > > > > spend far more less time doing it by myself....
> > > >
> > > > I was just picking at you. But, really, review of a such a simple patch
> > > > isn't that much of work (just a proper ret. value checks + proper casting)
> > > > and you know that those functions can be tricky for portability outside
> > > > POSIX (e.g. atoi and atol are POSIX and C99 but I haven't noticed that
> > > > atoll is POSIX only) and I have no way how to test it. So I assumed that
> > > > you would compile test it as well.
> > > >
> > > > My primary point was that we have certain problems with changes
> > > > reviewing. I remember several patches (mostly mine) were you gave an ACK
> > > > and then - after patch was long in - you have found out a compilation
> > > > issue on Windows which never popped out in my local builds.
> > > >
> > > > Anyway, this was really my fault in the first place - I will try to
> > > > check CONFORMING TO section of man pages more carefully.
> > > >
> > > > >
> > > > >
> > > > > >Check the attached patch.
> > > >
> > > > And it seems you have ignored it and reverted the change with log
> > > > message which makes quite a good sense:
> > > > "
> > > > [...]
> > > > 2. atoll converted to atol
> > > > [...]
> > > > "
> > >
> > > Btw. the change is not correct as well:
> > > @@ -812,7 +813,7 @@ GBool XRef::constructXRef() {
> > >
> > > // look for object
> > > } else if (isdigit((unsigned char)(*p))) {
> > > - num = (int)atoll(p);
> > > + num = (int)atol(p);
> > > if (num > 0) {
> > > do {
> > > ++p;
> > > @@ -822,7 +823,7 @@ GBool XRef::constructXRef() {
> > > ++p;
> > > } while (*p && isspace(*p));
> > > if (isdigit((unsigned char)*p)) {
> > > - gen = (int)atoll(p);
> > > + gen = (int)atol(p);
> > > do {
> > > ++p;
> > > } while (*p && isdigit((unsigned char)*p));
> > >
> > > This doesn't work on platforms where sizeof(int) == sizeof(long) == 32
> >
> > thx for mentioning this.
> >
>
> > first, i would like to clarify something. C99 is C standard, there is
>
> Sure, but this is a C function so this is only information I get from
> man pages.
>
> > no such thing as long long in official C++ standard so far (there will
> > be in C++0x though). however, all compilers support this extension but
> > not all the functions like atoll.
>
> That's why I posted the patch to test on Windows.
>
> >
> > the atoll change to atol was to have cvs compilation ready still in
> > compliance to pdf specification.
>
> > i wrote you an email about fixing it but nothing happened and that is
> > why i changed it to atol.
>
> Yes and I didn't have time for that then. There was no need to commit
> it...
>
> >
> > as i see it now, using atoi(atol) is ok, we only get MAX_INT as you
> > mention.
>
> Which ends up in an unreadable document, because there certainly is no
> MAX_INT object in there... So back to the point. Here is the refreshed
> patch. Do we want it?
>
>
> --
> Michal Hocko