> Date: Wed, 28 Apr 2010 13:34:01 +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 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':
> > > >>>>identifier not found
> > > >>>>5>..\..\src\xpdf\xpdf\XRef.cc(825) : error C3861: 'atoll':
> > > >>>>identifier not found
> > > >
> > > ><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 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.

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.

as i see it now, using atoi(atol) is ok, we only get MAX_INT as you mention.

/jozef


>
> >
> > Which we will understand 2 years later when we see a similar issue...
> >
> > Should I refresh the patch on top of your revert or we simply ended up
> > ignoring those objects with gen larger than 2B?
> >
> >
> > --
> > Michal Hocko
> >
> > ------------------------------------------------------------------------------
> > _______________________________________________
> > Pdfedit-devel mailing list
> > Pdfedit-devel@lists.sourceforge.net
> > https://lists.sourceforge.net/lists/listinfo/pdfedit-devel
>
> --
> Michal Hocko


The New Busy is not the too busy. Combine all your e-mail accounts with Hotmail. Get busy.