From: Michal H. <ms...@gm...> - 2010-04-19 18:50:52
|
On Sun, Apr 18, 2010 at 10:26:09PM +0000, Jozef Misutka wrote: > Update of /cvsroot/pdfedit/pdfedit/src/xpdf/xpdf > In directory sfp-cvsdas-2.v30.ch3.sourceforge.com:/tmp/cvs-serv17731 > > Modified Files: > SplashOutputDev.cc > Log Message: > > added null checks (otherwise crash on win32 test pdf file) This doesn't sound like a fix to me. How this can possibly happen? Is it just a bad code usage or a bug in the code? I don't like the change because it papers over maybe a real bug. Please revert the change and file a bug with the test pdf attached. Maybe we will find out that the crash is not windows specific. > > Index: SplashOutputDev.cc > =================================================================== > RCS file: /cvsroot/pdfedit/pdfedit/src/xpdf/xpdf/SplashOutputDev.cc,v > retrieving revision 1.6 > retrieving revision 1.7 > diff -u -d -r1.6 -r1.7 > --- SplashOutputDev.cc 11 Sep 2009 12:02:56 -0000 1.6 > +++ SplashOutputDev.cc 18 Apr 2010 22:26:06 -0000 1.7 > @@ -2553,6 +2553,8 @@ > const double *ctm; > > // restore state > + if (!transpGroupStack) > + return; > delete splash; This is a potential memory leak! > bitmap = transpGroupStack->origBitmap; > splash = transpGroupStack->origSplash; > @@ -2567,6 +2569,8 @@ > GBool isolated; > int tx, ty; > > + if (!transpGroupStack) > + return; > tx = transpGroupStack->tx; > ty = transpGroupStack->ty; > tBitmap = transpGroupStack->tBitmap; -- Michal Hocko |
From: Jozef M. <mis...@ho...> - 2010-04-19 21:25:06
|
---------------------------------------- > Date: Mon, 19 Apr 2010 20:50:40 +0200 > From: ms...@gm... > To: mis...@us... > Subject: Re: [Pdfedit-cvs] pdfedit/src/xpdf/xpdf SplashOutputDev.cc,1.6,1.7 > CC: pdf...@li... > > On Sun, Apr 18, 2010 at 10:26:09PM +0000, Jozef Misutka wrote: >> Update of /cvsroot/pdfedit/pdfedit/src/xpdf/xpdf >> In directory sfp-cvsdas-2.v30.ch3.sourceforge.com:/tmp/cvs-serv17731 >> >> Modified Files: >> SplashOutputDev.cc >> Log Message: >> >> added null checks (otherwise crash on win32 test pdf file) > > This doesn't sound like a fix to me. How this can possibly happen? Is it > just a bad code usage or a bug in the code? > > I don't like the change because it papers over maybe a real bug. > Please revert the change and file a bug with the test pdf attached. > Maybe we will find out that the crash is not windows specific. I do not think that reverting a potential fix is better than having something working in the cvs. It is easier to comment out a fix than to add it. i will properly file the issue later. jozef > >> >> Index: SplashOutputDev.cc >> =================================================================== >> RCS file: /cvsroot/pdfedit/pdfedit/src/xpdf/xpdf/SplashOutputDev.cc,v >> retrieving revision 1.6 >> retrieving revision 1.7 >> diff -u -d -r1.6 -r1.7 >> --- SplashOutputDev.cc 11 Sep 2009 12:02:56 -0000 1.6 >> +++ SplashOutputDev.cc 18 Apr 2010 22:26:06 -0000 1.7 >> @@ -2553,6 +2553,8 @@ >> const double *ctm; >> >> // restore state >> + if (!transpGroupStack) >> + return; >> delete splash; > > This is a potential memory leak! > >> bitmap = transpGroupStack->origBitmap; >> splash = transpGroupStack->origSplash; >> @@ -2567,6 +2569,8 @@ >> GBool isolated; >> int tx, ty; >> >> + if (!transpGroupStack) >> + return; >> tx = transpGroupStack->tx; >> ty = transpGroupStack->ty; >> tBitmap = transpGroupStack->tBitmap; > > -- > Michal Hocko > > ------------------------------------------------------------------------------ > Download Intel® Parallel Studio Eval > Try the new software tools for yourself. Speed compiling, find bugs > proactively, and fine-tune applications for parallel performance. > See why Intel Parallel Studio got high marks during beta. > http://p.sf.net/sfu/intel-sw-dev > _______________________________________________ > Pdfedit-devel mailing list > Pdf...@li... > https://lists.sourceforge.net/lists/listinfo/pdfedit-devel _________________________________________________________________ The New Busy is not the old busy. Search, chat and e-mail from your inbox. http://www.windowslive.com/campaign/thenewbusy?ocid=PID28326::T:WLMTAGL:ON:WL:en-US:WM_HMP:042010_3 |
From: Michal H. <ms...@gm...> - 2010-04-19 21:50:26
|
On Mon, Apr 19, 2010 at 09:20:33PM +0000, Jozef Misutka wrote: > > On Sun, Apr 18, 2010 at 10:26:09PM +0000, Jozef Misutka wrote: > >> Update of /cvsroot/pdfedit/pdfedit/src/xpdf/xpdf > >> In directory sfp-cvsdas-2.v30.ch3.sourceforge.com:/tmp/cvs-serv17731 > >> > >> Modified Files: > >> SplashOutputDev.cc > >> Log Message: > >> > >> added null checks (otherwise crash on win32 test pdf file) > > > > This doesn't sound like a fix to me. How this can possibly happen? Is it > > just a bad code usage or a bug in the code? > > > > I don't like the change because it papers over maybe a real bug. > > Please revert the change and file a bug with the test pdf attached. > > Maybe we will find out that the crash is not windows specific. > > I do not think that reverting a potential fix is better than having > something working in the cvs. This is a plain hack. There are more places where the variable is used and they are not checked. So you have basically turned off code paths which happened to crash rather than checked out why that code crashed. So I think that we should revert the change and properly find out the reason. I can help with that if I am able to reproduce (could you send the document?) > It is easier to comment out a fix than to add it. But this doesn't help us to figure out what the problem is. > > i will properly file the issue later. Why not now? > > > jozef > > > > >> > >> Index: SplashOutputDev.cc > >> =================================================================== > >> RCS file: /cvsroot/pdfedit/pdfedit/src/xpdf/xpdf/SplashOutputDev.cc,v > >> retrieving revision 1.6 > >> retrieving revision 1.7 > >> diff -u -d -r1.6 -r1.7 > >> --- SplashOutputDev.cc 11 Sep 2009 12:02:56 -0000 1.6 > >> +++ SplashOutputDev.cc 18 Apr 2010 22:26:06 -0000 1.7 > >> @@ -2553,6 +2553,8 @@ > >> const double *ctm; > >> > >> // restore state > >> + if (!transpGroupStack) > >> + return; > >> delete splash; > > > > This is a potential memory leak! > > > >> bitmap = transpGroupStack->origBitmap; > >> splash = transpGroupStack->origSplash; > >> @@ -2567,6 +2569,8 @@ > >> GBool isolated; > >> int tx, ty; > >> > >> + if (!transpGroupStack) > >> + return; > >> tx = transpGroupStack->tx; > >> ty = transpGroupStack->ty; > >> tBitmap = transpGroupStack->tBitmap; > > > > -- > > Michal Hocko > > > > ------------------------------------------------------------------------------ > > Download Intel? Parallel Studio Eval > > Try the new software tools for yourself. Speed compiling, find bugs > > proactively, and fine-tune applications for parallel performance. > > See why Intel Parallel Studio got high marks during beta. > > http://p.sf.net/sfu/intel-sw-dev > > _______________________________________________ > > Pdfedit-devel mailing list > > Pdf...@li... > > https://lists.sourceforge.net/lists/listinfo/pdfedit-devel > > _________________________________________________________________ > The New Busy is not the old busy. Search, chat and e-mail from your inbox. > http://www.windowslive.com/campaign/thenewbusy?ocid=PID28326::T:WLMTAGL:ON:WL:en-US:WM_HMP:042010_3 -- Michal Hocko |
From: Michal H. <ms...@gm...> - 2010-04-26 17:18:50
|
On Mon, Apr 19, 2010 at 11:50:18PM +0200, Michal Hocko wrote: > On Mon, Apr 19, 2010 at 09:20:33PM +0000, Jozef Misutka wrote: > > > On Sun, Apr 18, 2010 at 10:26:09PM +0000, Jozef Misutka wrote: > > >> Update of /cvsroot/pdfedit/pdfedit/src/xpdf/xpdf > > >> In directory sfp-cvsdas-2.v30.ch3.sourceforge.com:/tmp/cvs-serv17731 > > >> > > >> Modified Files: > > >> SplashOutputDev.cc > > >> Log Message: > > >> > > >> added null checks (otherwise crash on win32 test pdf file) > > > > > > This doesn't sound like a fix to me. How this can possibly happen? Is it > > > just a bad code usage or a bug in the code? > > > > > > I don't like the change because it papers over maybe a real bug. > > > Please revert the change and file a bug with the test pdf attached. > > > Maybe we will find out that the crash is not windows specific. > > > > I do not think that reverting a potential fix is better than having > > something working in the cvs. > > This is a plain hack. There are more places where the variable is used > and they are not checked. So you have basically turned off code paths > which happened to crash rather than checked out why that code crashed. So > I think that we should revert the change and properly find out the > reason. I can help with that if I am able to reproduce (could you send > the document?) What's up with this bug? Is it possible that it was caused by vitual methods mass caused by const cleanup? > > > It is easier to comment out a fix than to add it. > > But this doesn't help us to figure out what the problem is. > > > > > i will properly file the issue later. > > Why not now? Can you report it with the document attached to it? > > > > > > > jozef > > > > > > > >> > > >> Index: SplashOutputDev.cc > > >> =================================================================== > > >> RCS file: /cvsroot/pdfedit/pdfedit/src/xpdf/xpdf/SplashOutputDev.cc,v > > >> retrieving revision 1.6 > > >> retrieving revision 1.7 > > >> diff -u -d -r1.6 -r1.7 > > >> --- SplashOutputDev.cc 11 Sep 2009 12:02:56 -0000 1.6 > > >> +++ SplashOutputDev.cc 18 Apr 2010 22:26:06 -0000 1.7 > > >> @@ -2553,6 +2553,8 @@ > > >> const double *ctm; > > >> > > >> // restore state > > >> + if (!transpGroupStack) > > >> + return; > > >> delete splash; > > > > > > This is a potential memory leak! > > > > > >> bitmap = transpGroupStack->origBitmap; > > >> splash = transpGroupStack->origSplash; > > >> @@ -2567,6 +2569,8 @@ > > >> GBool isolated; > > >> int tx, ty; > > >> > > >> + if (!transpGroupStack) > > >> + return; > > >> tx = transpGroupStack->tx; > > >> ty = transpGroupStack->ty; > > >> tBitmap = transpGroupStack->tBitmap; > > > > > > -- > > > Michal Hocko > > > > > > ------------------------------------------------------------------------------ > > > Download Intel? Parallel Studio Eval > > > Try the new software tools for yourself. Speed compiling, find bugs > > > proactively, and fine-tune applications for parallel performance. > > > See why Intel Parallel Studio got high marks during beta. > > > http://p.sf.net/sfu/intel-sw-dev > > > _______________________________________________ > > > Pdfedit-devel mailing list > > > Pdf...@li... > > > https://lists.sourceforge.net/lists/listinfo/pdfedit-devel > > > > _________________________________________________________________ > > The New Busy is not the old busy. Search, chat and e-mail from your inbox. > > http://www.windowslive.com/campaign/thenewbusy?ocid=PID28326::T:WLMTAGL:ON:WL:en-US:WM_HMP:042010_3 > > -- > Michal Hocko > > ------------------------------------------------------------------------------ > Download Intel® Parallel Studio Eval > Try the new software tools for yourself. Speed compiling, find bugs > proactively, and fine-tune applications for parallel performance. > See why Intel Parallel Studio got high marks during beta. > http://p.sf.net/sfu/intel-sw-dev > _______________________________________________ > Pdfedit-devel mailing list > Pdf...@li... > https://lists.sourceforge.net/lists/listinfo/pdfedit-devel -- Michal Hocko |
From: Jozef M. <mis...@ho...> - 2010-04-27 11:37:31
|
On Mon, 26 Apr 2010 20:29:05 +0300, Michal Hocko <ms...@gm...> 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 >> >> >> >> >> >On Mon, Apr 19, 2010 at 11:50:18PM +0200, Michal Hocko wrote: >> >>On Mon, Apr 19, 2010 at 09:20:33PM +0000, Jozef Misutka wrote: >> >>> > On Sun, Apr 18, 2010 at 10:26:09PM +0000, Jozef Misutka wrote: >> >>> >> Update of /cvsroot/pdfedit/pdfedit/src/xpdf/xpdf >> >>> >> In directory >> >>sfp-cvsdas-2.v30.ch3.sourceforge.com:/tmp/cvs-serv17731 >> >>> >> >> >>> >> Modified Files: >> >>> >> SplashOutputDev.cc >> >>> >> Log Message: >> >>> >> >> >>> >> added null checks (otherwise crash on win32 test pdf file) >> >>> > >> >>> > This doesn't sound like a fix to me. How this can possibly >> >>happen? Is it >> >>> > just a bad code usage or a bug in the code? >> >>> > >> >>> > I don't like the change because it papers over maybe a real bug. >> >>> > Please revert the change and file a bug with the test pdf >> attached. >> >>> > Maybe we will find out that the crash is not windows specific. >> >>> >> >>> I do not think that reverting a potential fix is better than having >> >>> something working in the cvs. >> >> >> >>This is a plain hack. There are more places where the variable is used >> >>and they are not checked. So you have basically turned off code paths >> >>which happened to crash rather than checked out why that code >> >>crashed. So >> >>I think that we should revert the change and properly find out the >> >>reason. I can help with that if I am able to reproduce (could you send >> >>the document?) >> > >> >What's up with this bug? Is it possible that it was caused by vitual >> >methods mass caused by const cleanup? >> > >> >> >> >>> It is easier to comment out a fix than to add it. >> >> >> >>But this doesn't help us to figure out what the problem is. >> >> >> >>> >> >>> i will properly file the issue later. >> >> >> >>Why not now? >> > >> >Can you report it with the document attached to it? >> > >> >> >> >>> >> >>> >> >>> jozef >> >>> >> >>> > >> >>> >> >> >>> >> Index: SplashOutputDev.cc >> >>> >> >> =================================================================== >> >>> >> RCS file: >> >>/cvsroot/pdfedit/pdfedit/src/xpdf/xpdf/SplashOutputDev.cc,v >> >>> >> retrieving revision 1.6 >> >>> >> retrieving revision 1.7 >> >>> >> diff -u -d -r1.6 -r1.7 >> >>> >> --- SplashOutputDev.cc 11 Sep 2009 12:02:56 -0000 1.6 >> >>> >> +++ SplashOutputDev.cc 18 Apr 2010 22:26:06 -0000 1.7 >> >>> >> @@ -2553,6 +2553,8 @@ >> >>> >> const double *ctm; >> >>> >> >> >>> >> // restore state >> >>> >> + if (!transpGroupStack) >> >>> >> + return; >> >>> >> delete splash; >> >>> > >> >>> > This is a potential memory leak! >> >>> > >> >>> >> bitmap = transpGroupStack->origBitmap; >> >>> >> splash = transpGroupStack->origSplash; >> >>> >> @@ -2567,6 +2569,8 @@ >> >>> >> GBool isolated; >> >>> >> int tx, ty; >> >>> >> >> >>> >> + if (!transpGroupStack) >> >>> >> + return; >> >>> >> tx = transpGroupStack->tx; >> >>> >> ty = transpGroupStack->ty; >> >>> >> tBitmap = transpGroupStack->tBitmap; >> >>> > >> >>> > -- >> >>> > Michal Hocko >> >>> > >> >>> > >> ------------------------------------------------------------------------------ >> >>> > Download Intel? Parallel Studio Eval >> >>> > Try the new software tools for yourself. Speed compiling, find >> bugs >> >>> > proactively, and fine-tune applications for parallel performance. >> >>> > See why Intel Parallel Studio got high marks during beta. >> >>> > http://p.sf.net/sfu/intel-sw-dev >> >>> > _______________________________________________ >> >>> > Pdfedit-devel mailing list >> >>> > Pdf...@li... >> >>> > https://lists.sourceforge.net/lists/listinfo/pdfedit-devel >> >>> >> >>> _________________________________________________________________ >> >>> The New Busy is not the old busy. Search, chat and e-mail from >> >>your inbox. >> >>> >> http://www.windowslive.com/campaign/thenewbusy?ocid=PID28326::T:WLMTAGL:ON:WL:en-US:WM_HMP:042010_3 >> >> >> >>-- >> >>Michal Hocko >> >> >> >>------------------------------------------------------------------------------ >> >>Download Intel® Parallel Studio Eval >> >>Try the new software tools for yourself. Speed compiling, find bugs >> >>proactively, and fine-tune applications for parallel performance. >> >>See why Intel Parallel Studio got high marks during beta. >> >>http://p.sf.net/sfu/intel-sw-dev >> >>_______________________________________________ >> >>Pdfedit-devel mailing list >> >>Pdf...@li... >> >>https://lists.sourceforge.net/lists/listinfo/pdfedit-devel >> > >> >> >> -- >> Using Opera's revolutionary e-mail client: http://www.opera.com/mail/ > -- Using Opera's revolutionary e-mail client: http://www.opera.com/mail/ |
From: Michal H. <ms...@gm...> - 2010-04-27 12:52:46
Attachments:
xpdf_replace_atoll_by_c99alternative.patch
|
On Tue, Apr 27, 2010 at 01:37:13PM +0300, Jozef Misutka wrote: > On Mon, 26 Apr 2010 20:29:05 +0300, Michal Hocko <ms...@gm...> 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> Check the attached patch. -- Michal Hocko |
From: Jozef M. <mis...@ho...> - 2010-04-27 18:07:49
|
On Tue, 27 Apr 2010 15:52:35 +0300, Michal Hocko <ms...@gm...> 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 <ms...@gm...> >> 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.... > Check the attached patch. > -- Using Opera's revolutionary e-mail client: http://www.opera.com/mail/ |
From: Michal H. <ms...@gm...> - 2010-04-27 21:23:30
|
On Tue, Apr 27, 2010 at 08:07:35PM +0300, Jozef Misutka wrote: > On Tue, 27 Apr 2010 15:52:35 +0300, Michal Hocko <ms...@gm...> 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 > >><ms...@gm...> 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? Well, this is not necessary as I am able to compile for all supported platforms, which is not the case here. Actually, you are the only one who can compile for Windows. > > > >Check the attached patch. > > > > > -- > Using Opera's revolutionary e-mail client: http://www.opera.com/mail/ -- Michal Hocko |
From: Jozef M. <mis...@ho...> - 2010-04-28 13:11:36
|
> Date: Wed, 28 Apr 2010 13:34:01 +0200 > From: ms...@gm... > To: mis...@ho... > CC: pdf...@li... > 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 <ms...@gm...> 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 > > > >><ms...@gm...> 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 > > Pdf...@li... > > 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. http://www.windowslive.com/campaign/thenewbusy?tile=multiaccount&ocid=PID28326::T:WLMTAGL:ON:WL:en-US:WM_HMP:042010_4 |
From: Michal H. <ms...@gm...> - 2010-04-28 14:06:14
Attachments:
xpdf_replace_atoll_by_c99alternative.patch
|
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 <ms...@gm...> 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 > > > > >><ms...@gm...> 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 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 |
From: Jozef M. <mis...@ho...> - 2010-04-28 14:28:21
|
Error 4 error C3861: 'strtoll': identifier not found pdfedit\src\xpdf\xpdf\XRef.cc 816 xpdf :( > Date: Wed, 28 Apr 2010 16:06:07 +0200 > From: ms...@gm... > To: mis...@ho... > CC: pdf...@li... > 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 <ms...@gm...> 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 > > > > > >><ms...@gm...> 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 > > 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 _________________________________________________________________ Hotmail has tools for the New Busy. Search, chat and e-mail from your inbox. http://www.windowslive.com/campaign/thenewbusy?ocid=PID28326::T:WLMTAGL:ON:WL:en-US:WM_HMP:042010_1 |
From: Michal H. <ms...@gm...> - 2010-04-28 14:46:20
Attachments:
xpdf_replace_atoll_by_c99alternative.patch
|
On Wed, Apr 28, 2010 at 02:28:14PM +0000, Jozef Misutka wrote: > > Error 4 error C3861: 'strtoll': identifier not found pdfedit\src\xpdf\xpdf\XRef.cc 816 xpdf OK, then the last attempt, which should have been tried at the very beginning - but well I was kind of blind to it - and use usigned version (I cannot see any for int but this one should be OK as well). Could you try the attached patch. I have tested it with the mangled document you have originally attached to the bugzilla and it works. > > :( > > > > Date: Wed, 28 Apr 2010 16:06:07 +0200 > > From: ms...@gm... > > To: mis...@ho... > > CC: pdf...@li... > > 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 <ms...@gm...> 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 > > > > > > >><ms...@gm...> 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 > > > > 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 > > _________________________________________________________________ > Hotmail has tools for the New Busy. Search, chat and e-mail from your inbox. > http://www.windowslive.com/campaign/thenewbusy?ocid=PID28326::T:WLMTAGL:ON:WL:en-US:WM_HMP:042010_1 -- Michal Hocko |
From: Jozef M. <mis...@ho...> - 2010-04-28 19:40:52
|
ACK, thx! ---------------------------------------- > Date: Wed, 28 Apr 2010 16:46:12 +0200 > From: ms...@gm... > To: mis...@ho... > CC: pdf...@li... > Subject: Re: [Pdfedit-cvs] pdfedit/src/xpdf/xpdf SplashOutputDev.cc,1.6,1.7 > > On Wed, Apr 28, 2010 at 02:28:14PM +0000, Jozef Misutka wrote: >> >> Error 4 error C3861: 'strtoll': identifier not found pdfedit\src\xpdf\xpdf\XRef.cc 816 xpdf > > OK, then the last attempt, which should have been tried at the very > beginning - but well I was kind of blind to it - and use usigned > version (I cannot see any for int but this one should be OK as well). > Could you try the attached patch. > > I have tested it with the mangled document you have originally attached > to the bugzilla and it works. > >> >> :( >> >> >>> Date: Wed, 28 Apr 2010 16:06:07 +0200 >>> From: ms...@gm... >>> To: mis...@ho... >>> CC: pdf...@li... >>> 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 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 >>>>>>>>> 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 >>>>>>>> >>>>>>>> >>>>>>>>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). >>>>>>>> >>>>>>> >>>>>>> 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 >> >> _________________________________________________________________ >> Hotmail has tools for the New Busy. Search, chat and e-mail from your inbox. >> http://www.windowslive.com/campaign/thenewbusy?ocid=PID28326::T:WLMTAGL:ON:WL:en-US:WM_HMP:042010_1 > > -- > Michal Hocko _________________________________________________________________ Hotmail is redefining busy with tools for the New Busy. Get more from your inbox. http://www.windowslive.com/campaign/thenewbusy?ocid=PID28326::T:WLMTAGL:ON:WL:en-US:WM_HMP:042010_2 |
From: Michal H. <ms...@gm...> - 2010-05-03 20:36:19
|
On Mon, Apr 19, 2010 at 08:50:40PM +0200, Michal Hocko wrote: > On Sun, Apr 18, 2010 at 10:26:09PM +0000, Jozef Misutka wrote: > > Update of /cvsroot/pdfedit/pdfedit/src/xpdf/xpdf > > In directory sfp-cvsdas-2.v30.ch3.sourceforge.com:/tmp/cvs-serv17731 > > > > Modified Files: > > SplashOutputDev.cc > > Log Message: > > > > added null checks (otherwise crash on win32 test pdf file) > > This doesn't sound like a fix to me. How this can possibly happen? Is it > just a bad code usage or a bug in the code? > > I don't like the change because it papers over maybe a real bug. > Please revert the change and file a bug with the test pdf attached. > Maybe we will find out that the crash is not windows specific. Please revert this change. > > > > > Index: SplashOutputDev.cc > > =================================================================== > > RCS file: /cvsroot/pdfedit/pdfedit/src/xpdf/xpdf/SplashOutputDev.cc,v > > retrieving revision 1.6 > > retrieving revision 1.7 > > diff -u -d -r1.6 -r1.7 > > --- SplashOutputDev.cc 11 Sep 2009 12:02:56 -0000 1.6 > > +++ SplashOutputDev.cc 18 Apr 2010 22:26:06 -0000 1.7 > > @@ -2553,6 +2553,8 @@ > > const double *ctm; > > > > // restore state > > + if (!transpGroupStack) > > + return; > > delete splash; > > This is a potential memory leak! > > > bitmap = transpGroupStack->origBitmap; > > splash = transpGroupStack->origSplash; > > @@ -2567,6 +2569,8 @@ > > GBool isolated; > > int tx, ty; > > > > + if (!transpGroupStack) > > + return; > > tx = transpGroupStack->tx; > > ty = transpGroupStack->ty; > > tBitmap = transpGroupStack->tBitmap; > > -- > Michal Hocko > > ------------------------------------------------------------------------------ > Download Intel® Parallel Studio Eval > Try the new software tools for yourself. Speed compiling, find bugs > proactively, and fine-tune applications for parallel performance. > See why Intel Parallel Studio got high marks during beta. > http://p.sf.net/sfu/intel-sw-dev > _______________________________________________ > Pdfedit-devel mailing list > Pdf...@li... > https://lists.sourceforge.net/lists/listinfo/pdfedit-devel -- Michal Hocko |
From: Jozef M. <mis...@ho...> - 2010-05-03 20:38:08
|
On Mon, 03 May 2010 22:36:09 +0200, Michal Hocko <ms...@gm...> wrote: > On Mon, Apr 19, 2010 at 08:50:40PM +0200, Michal Hocko wrote: >> On Sun, Apr 18, 2010 at 10:26:09PM +0000, Jozef Misutka wrote: >> > Update of /cvsroot/pdfedit/pdfedit/src/xpdf/xpdf >> > In directory sfp-cvsdas-2.v30.ch3.sourceforge.com:/tmp/cvs-serv17731 >> > >> > Modified Files: >> > SplashOutputDev.cc >> > Log Message: >> > >> > added null checks (otherwise crash on win32 test pdf file) >> >> This doesn't sound like a fix to me. How this can possibly happen? Is it >> just a bad code usage or a bug in the code? >> >> I don't like the change because it papers over maybe a real bug. >> Please revert the change and file a bug with the test pdf attached. >> Maybe we will find out that the crash is not windows specific. > > Please revert this change. i do not see why, we have already discussed it. > >> >> > >> > Index: SplashOutputDev.cc >> > =================================================================== >> > RCS file: /cvsroot/pdfedit/pdfedit/src/xpdf/xpdf/SplashOutputDev.cc,v >> > retrieving revision 1.6 >> > retrieving revision 1.7 >> > diff -u -d -r1.6 -r1.7 >> > --- SplashOutputDev.cc 11 Sep 2009 12:02:56 -0000 1.6 >> > +++ SplashOutputDev.cc 18 Apr 2010 22:26:06 -0000 1.7 >> > @@ -2553,6 +2553,8 @@ >> > const double *ctm; >> > >> > // restore state >> > + if (!transpGroupStack) >> > + return; >> > delete splash; >> >> This is a potential memory leak! >> >> > bitmap = transpGroupStack->origBitmap; >> > splash = transpGroupStack->origSplash; >> > @@ -2567,6 +2569,8 @@ >> > GBool isolated; >> > int tx, ty; >> > >> > + if (!transpGroupStack) >> > + return; >> > tx = transpGroupStack->tx; >> > ty = transpGroupStack->ty; >> > tBitmap = transpGroupStack->tBitmap; >> >> -- >> Michal Hocko >> >> ------------------------------------------------------------------------------ >> Download Intel® Parallel Studio Eval >> Try the new software tools for yourself. Speed compiling, find bugs >> proactively, and fine-tune applications for parallel performance. >> See why Intel Parallel Studio got high marks during beta. >> http://p.sf.net/sfu/intel-sw-dev >> _______________________________________________ >> Pdfedit-devel mailing list >> Pdf...@li... >> https://lists.sourceforge.net/lists/listinfo/pdfedit-devel > -- Using Opera's revolutionary e-mail client: http://www.opera.com/mail/ |
From: Michal H. <ms...@gm...> - 2010-05-03 20:44:00
|
On Mon, May 03, 2010 at 10:37:54PM +0200, Jozef Misutka wrote: > On Mon, 03 May 2010 22:36:09 +0200, Michal Hocko <ms...@gm...> wrote: > > >On Mon, Apr 19, 2010 at 08:50:40PM +0200, Michal Hocko wrote: > >>On Sun, Apr 18, 2010 at 10:26:09PM +0000, Jozef Misutka wrote: > >>> Update of /cvsroot/pdfedit/pdfedit/src/xpdf/xpdf > >>> In directory sfp-cvsdas-2.v30.ch3.sourceforge.com:/tmp/cvs-serv17731 > >>> > >>> Modified Files: > >>> SplashOutputDev.cc > >>> Log Message: > >>> > >>> added null checks (otherwise crash on win32 test pdf file) > >> > >>This doesn't sound like a fix to me. How this can possibly happen? Is it > >>just a bad code usage or a bug in the code? > >> > >>I don't like the change because it papers over maybe a real bug. > >>Please revert the change and file a bug with the test pdf attached. > >>Maybe we will find out that the crash is not windows specific. > > > >Please revert this change. > > i do not see why, we have already discussed it. What about arguments mentioned in this email. You have never answered any of of them. The only thing that came from you is that you don't have a document which caused the issue. Are we talking about the same thing? It doesn't seem so... > >>> Index: SplashOutputDev.cc > >>> =================================================================== > >>> RCS file: /cvsroot/pdfedit/pdfedit/src/xpdf/xpdf/SplashOutputDev.cc,v > >>> retrieving revision 1.6 > >>> retrieving revision 1.7 > >>> diff -u -d -r1.6 -r1.7 > >>> --- SplashOutputDev.cc 11 Sep 2009 12:02:56 -0000 1.6 > >>> +++ SplashOutputDev.cc 18 Apr 2010 22:26:06 -0000 1.7 > >>> @@ -2553,6 +2553,8 @@ > >>> const double *ctm; > >>> > >>> // restore state > >>> + if (!transpGroupStack) > >>> + return; > >>> delete splash; > >> > >>This is a potential memory leak! > >> > >>> bitmap = transpGroupStack->origBitmap; > >>> splash = transpGroupStack->origSplash; > >>> @@ -2567,6 +2569,8 @@ > >>> GBool isolated; > >>> int tx, ty; > >>> > >>> + if (!transpGroupStack) > >>> + return; > >>> tx = transpGroupStack->tx; > >>> ty = transpGroupStack->ty; > >>> tBitmap = transpGroupStack->tBitmap; -- Michal Hocko |
From: Jozef M. <mis...@ho...> - 2010-05-03 20:46:28
|
On Mon, 03 May 2010 22:43:53 +0200, Michal Hocko <ms...@gm...> wrote: > On Mon, May 03, 2010 at 10:37:54PM +0200, Jozef Misutka wrote: >> On Mon, 03 May 2010 22:36:09 +0200, Michal Hocko <ms...@gm...> >> wrote: >> >> >On Mon, Apr 19, 2010 at 08:50:40PM +0200, Michal Hocko wrote: >> >>On Sun, Apr 18, 2010 at 10:26:09PM +0000, Jozef Misutka wrote: >> >>> Update of /cvsroot/pdfedit/pdfedit/src/xpdf/xpdf >> >>> In directory sfp-cvsdas-2.v30.ch3.sourceforge.com:/tmp/cvs-serv17731 >> >>> >> >>> Modified Files: >> >>> SplashOutputDev.cc >> >>> Log Message: >> >>> >> >>> added null checks (otherwise crash on win32 test pdf file) >> >> >> >>This doesn't sound like a fix to me. How this can possibly happen? Is >> it >> >>just a bad code usage or a bug in the code? >> >> >> >>I don't like the change because it papers over maybe a real bug. >> >>Please revert the change and file a bug with the test pdf attached. >> >>Maybe we will find out that the crash is not windows specific. >> > >> >Please revert this change. >> >> i do not see why, we have already discussed it. > > What about arguments mentioned in this email. You have never answered > any of of them. > The only thing that came from you is that you don't have a document > which caused the issue. > Are we talking about the same thing? It doesn't seem so... arguments that we will rather wait for a crash than use this code? sorry, do not accept. jozef > >> >>> Index: SplashOutputDev.cc >> >>> =================================================================== >> >>> RCS file: >> /cvsroot/pdfedit/pdfedit/src/xpdf/xpdf/SplashOutputDev.cc,v >> >>> retrieving revision 1.6 >> >>> retrieving revision 1.7 >> >>> diff -u -d -r1.6 -r1.7 >> >>> --- SplashOutputDev.cc 11 Sep 2009 12:02:56 -0000 1.6 >> >>> +++ SplashOutputDev.cc 18 Apr 2010 22:26:06 -0000 1.7 >> >>> @@ -2553,6 +2553,8 @@ >> >>> const double *ctm; >> >>> >> >>> // restore state >> >>> + if (!transpGroupStack) >> >>> + return; >> >>> delete splash; >> >> >> >>This is a potential memory leak! >> >> >> >>> bitmap = transpGroupStack->origBitmap; >> >>> splash = transpGroupStack->origSplash; >> >>> @@ -2567,6 +2569,8 @@ >> >>> GBool isolated; >> >>> int tx, ty; >> >>> >> >>> + if (!transpGroupStack) >> >>> + return; >> >>> tx = transpGroupStack->tx; >> >>> ty = transpGroupStack->ty; >> >>> tBitmap = transpGroupStack->tBitmap; > -- Using Opera's revolutionary e-mail client: http://www.opera.com/mail/ |
From: Michal H. <ms...@gm...> - 2010-05-03 20:56:38
|
On Mon, May 03, 2010 at 10:46:13PM +0200, Jozef Misutka wrote: > On Mon, 03 May 2010 22:43:53 +0200, Michal Hocko <ms...@gm...> wrote: > > >On Mon, May 03, 2010 at 10:37:54PM +0200, Jozef Misutka wrote: > >>On Mon, 03 May 2010 22:36:09 +0200, Michal Hocko > >><ms...@gm...> wrote: > >> > >>>On Mon, Apr 19, 2010 at 08:50:40PM +0200, Michal Hocko wrote: > >>>>On Sun, Apr 18, 2010 at 10:26:09PM +0000, Jozef Misutka wrote: > >>>>> Update of /cvsroot/pdfedit/pdfedit/src/xpdf/xpdf > >>>>> In directory sfp-cvsdas-2.v30.ch3.sourceforge.com:/tmp/cvs-serv17731 > >>>>> > >>>>> Modified Files: > >>>>> SplashOutputDev.cc > >>>>> Log Message: > >>>>> > >>>>> added null checks (otherwise crash on win32 test pdf file) > >>>> > >>>>This doesn't sound like a fix to me. How this can possibly > >>happen? Is it > >>>>just a bad code usage or a bug in the code? > >>>> > >>>>I don't like the change because it papers over maybe a real bug. > >>>>Please revert the change and file a bug with the test pdf attached. > >>>>Maybe we will find out that the crash is not windows specific. > >>> > >>>Please revert this change. > >> > >>i do not see why, we have already discussed it. > > > >What about arguments mentioned in this email. You have never answered > >any of of them. > >The only thing that came from you is that you don't have a document > >which caused the issue. > >Are we talking about the same thing? It doesn't seem so... > > arguments that we will rather wait for a crash than use this code? > sorry, do not accept. How do you want to debug that without any crash? Please note that we have never seen any, except for the one which you cannot reproduce anymore and which could be a side effect of the const rework which has been fixed recently. This your change has my NACK and I will revert it if you do not do it. Sorry this is just not acceptable. We don't have to do hacks like that. Feel free to assign any bug report caused by this SEGV. > > jozef > > > > >>>>> Index: SplashOutputDev.cc > >>>>> =================================================================== > >>>>> RCS file: > >>/cvsroot/pdfedit/pdfedit/src/xpdf/xpdf/SplashOutputDev.cc,v > >>>>> retrieving revision 1.6 > >>>>> retrieving revision 1.7 > >>>>> diff -u -d -r1.6 -r1.7 > >>>>> --- SplashOutputDev.cc 11 Sep 2009 12:02:56 -0000 1.6 > >>>>> +++ SplashOutputDev.cc 18 Apr 2010 22:26:06 -0000 1.7 > >>>>> @@ -2553,6 +2553,8 @@ > >>>>> const double *ctm; > >>>>> > >>>>> // restore state > >>>>> + if (!transpGroupStack) > >>>>> + return; > >>>>> delete splash; > >>>> > >>>>This is a potential memory leak! > >>>> > >>>>> bitmap = transpGroupStack->origBitmap; > >>>>> splash = transpGroupStack->origSplash; > >>>>> @@ -2567,6 +2569,8 @@ > >>>>> GBool isolated; > >>>>> int tx, ty; > >>>>> > >>>>> + if (!transpGroupStack) > >>>>> + return; > >>>>> tx = transpGroupStack->tx; > >>>>> ty = transpGroupStack->ty; > >>>>> tBitmap = transpGroupStack->tBitmap; > > > > > -- > Using Opera's revolutionary e-mail client: http://www.opera.com/mail/ -- Michal Hocko |
From: Jozef M. <mis...@ho...> - 2010-05-04 05:27:08
|
On Mon, 03 May 2010 22:56:30 +0200, Michal Hocko <ms...@gm...> wrote: > On Mon, May 03, 2010 at 10:46:13PM +0200, Jozef Misutka wrote: >> On Mon, 03 May 2010 22:43:53 +0200, Michal Hocko <ms...@gm...> >> wrote: >> >> >On Mon, May 03, 2010 at 10:37:54PM +0200, Jozef Misutka wrote: >> >>On Mon, 03 May 2010 22:36:09 +0200, Michal Hocko >> >><ms...@gm...> wrote: >> >> >> >>>On Mon, Apr 19, 2010 at 08:50:40PM +0200, Michal Hocko wrote: >> >>>>On Sun, Apr 18, 2010 at 10:26:09PM +0000, Jozef Misutka wrote: >> >>>>> Update of /cvsroot/pdfedit/pdfedit/src/xpdf/xpdf >> >>>>> In directory >> sfp-cvsdas-2.v30.ch3.sourceforge.com:/tmp/cvs-serv17731 >> >>>>> >> >>>>> Modified Files: >> >>>>> SplashOutputDev.cc >> >>>>> Log Message: >> >>>>> >> >>>>> added null checks (otherwise crash on win32 test pdf file) >> >>>> >> >>>>This doesn't sound like a fix to me. How this can possibly >> >>happen? Is it >> >>>>just a bad code usage or a bug in the code? >> >>>> >> >>>>I don't like the change because it papers over maybe a real bug. >> >>>>Please revert the change and file a bug with the test pdf attached. >> >>>>Maybe we will find out that the crash is not windows specific. >> >>> >> >>>Please revert this change. >> >> >> >>i do not see why, we have already discussed it. >> > >> >What about arguments mentioned in this email. You have never answered >> >any of of them. >> >The only thing that came from you is that you don't have a document >> >which caused the issue. >> >Are we talking about the same thing? It doesn't seem so... >> >> arguments that we will rather wait for a crash than use this code? >> sorry, do not accept. > > How do you want to debug that without any crash? > Please note that we have never seen any, except for the one which you > cannot reproduce anymore and which could be a side effect of the const > rework which has been fixed recently. i canot find the file now, i am testing pdf on 100> files sometimes so sadly but it can happen. > > This your change has my NACK and I will revert it if you do not do it. > Sorry this is just not acceptable. We don't have to do hacks like that. > Feel free to assign any bug report caused by this SEGV. again, you prefer crash and debugging over simply not null checks. something imho crazy; however, xpdf is your code so you rule here. > >> >> jozef >> >> > >> >>>>> Index: SplashOutputDev.cc >> >>>>> >> =================================================================== >> >>>>> RCS file: >> >>/cvsroot/pdfedit/pdfedit/src/xpdf/xpdf/SplashOutputDev.cc,v >> >>>>> retrieving revision 1.6 >> >>>>> retrieving revision 1.7 >> >>>>> diff -u -d -r1.6 -r1.7 >> >>>>> --- SplashOutputDev.cc 11 Sep 2009 12:02:56 -0000 1.6 >> >>>>> +++ SplashOutputDev.cc 18 Apr 2010 22:26:06 -0000 1.7 >> >>>>> @@ -2553,6 +2553,8 @@ >> >>>>> const double *ctm; >> >>>>> >> >>>>> // restore state >> >>>>> + if (!transpGroupStack) >> >>>>> + return; >> >>>>> delete splash; >> >>>> >> >>>>This is a potential memory leak! >> >>>> >> >>>>> bitmap = transpGroupStack->origBitmap; >> >>>>> splash = transpGroupStack->origSplash; >> >>>>> @@ -2567,6 +2569,8 @@ >> >>>>> GBool isolated; >> >>>>> int tx, ty; >> >>>>> >> >>>>> + if (!transpGroupStack) >> >>>>> + return; >> >>>>> tx = transpGroupStack->tx; >> >>>>> ty = transpGroupStack->ty; >> >>>>> tBitmap = transpGroupStack->tBitmap; >> > >> >> >> -- >> Using Opera's revolutionary e-mail client: http://www.opera.com/mail/ > -- Using Opera's revolutionary e-mail client: http://www.opera.com/mail/ |
From: Michal H. <ms...@gm...> - 2010-05-04 07:04:12
|
On Tue, May 04, 2010 at 07:26:54AM +0200, Jozef Misutka wrote: > On Mon, 03 May 2010 22:56:30 +0200, Michal Hocko <ms...@gm...> wrote: > > >On Mon, May 03, 2010 at 10:46:13PM +0200, Jozef Misutka wrote: > >>On Mon, 03 May 2010 22:43:53 +0200, Michal Hocko > >><ms...@gm...> wrote: > >> > >>>On Mon, May 03, 2010 at 10:37:54PM +0200, Jozef Misutka wrote: > >>>>On Mon, 03 May 2010 22:36:09 +0200, Michal Hocko > >>>><ms...@gm...> wrote: > >>>> > >>>>>On Mon, Apr 19, 2010 at 08:50:40PM +0200, Michal Hocko wrote: > >>>>>>On Sun, Apr 18, 2010 at 10:26:09PM +0000, Jozef Misutka wrote: > >>>>>>> Update of /cvsroot/pdfedit/pdfedit/src/xpdf/xpdf > >>>>>>> In directory > >>sfp-cvsdas-2.v30.ch3.sourceforge.com:/tmp/cvs-serv17731 > >>>>>>> > >>>>>>> Modified Files: > >>>>>>> SplashOutputDev.cc > >>>>>>> Log Message: > >>>>>>> > >>>>>>> added null checks (otherwise crash on win32 test pdf file) > >>>>>> > >>>>>>This doesn't sound like a fix to me. How this can possibly > >>>>happen? Is it > >>>>>>just a bad code usage or a bug in the code? > >>>>>> > >>>>>>I don't like the change because it papers over maybe a real bug. > >>>>>>Please revert the change and file a bug with the test pdf attached. > >>>>>>Maybe we will find out that the crash is not windows specific. > >>>>> > >>>>>Please revert this change. > >>>> > >>>>i do not see why, we have already discussed it. > >>> > >>>What about arguments mentioned in this email. You have never answered > >>>any of of them. > >>>The only thing that came from you is that you don't have a document > >>>which caused the issue. > >>>Are we talking about the same thing? It doesn't seem so... > >> > >>arguments that we will rather wait for a crash than use this code? > >>sorry, do not accept. > > > >How do you want to debug that without any crash? > >Please note that we have never seen any, except for the one which you > >cannot reproduce anymore and which could be a side effect of the const > >rework which has been fixed recently. > > i canot find the file now, i am testing pdf on 100> files sometimes > so sadly but it can happen. > > > > >This your change has my NACK and I will revert it if you do not do it. > >Sorry this is just not acceptable. We don't have to do hacks like that. > >Feel free to assign any bug report caused by this SEGV. > > again, you prefer crash and debugging over simply not null checks. This is how the code is written and your change didn't check other places where the transpGroupStack is used. So this is a plain hack - sorry that I repeat myself. > something imho crazy; however, xpdf is your code so you rule here. You apparently don't understand the code path or how it could happen. I don't know that as well but I rather want to be conservative than add something that none of us understands. If we happen to crash there then we can debug it. Otherwise there might be some hidden error poping out somewhere else - nothing I would like to debug... So I don't see anything crazy about that. -- Michal Hocko |
From: Michal H. <ms...@gm...> - 2010-05-04 07:16:07
|
On Tue, May 04, 2010 at 09:04:04AM +0200, Michal Hocko wrote: > On Tue, May 04, 2010 at 07:26:54AM +0200, Jozef Misutka wrote: > > On Mon, 03 May 2010 22:56:30 +0200, Michal Hocko <ms...@gm...> wrote: > > > > >On Mon, May 03, 2010 at 10:46:13PM +0200, Jozef Misutka wrote: > > >>On Mon, 03 May 2010 22:43:53 +0200, Michal Hocko > > >><ms...@gm...> wrote: > > >> > > >>>On Mon, May 03, 2010 at 10:37:54PM +0200, Jozef Misutka wrote: > > >>>>On Mon, 03 May 2010 22:36:09 +0200, Michal Hocko > > >>>><ms...@gm...> wrote: > > >>>> > > >>>>>On Mon, Apr 19, 2010 at 08:50:40PM +0200, Michal Hocko wrote: > > >>>>>>On Sun, Apr 18, 2010 at 10:26:09PM +0000, Jozef Misutka wrote: > > >>>>>>> Update of /cvsroot/pdfedit/pdfedit/src/xpdf/xpdf > > >>>>>>> In directory > > >>sfp-cvsdas-2.v30.ch3.sourceforge.com:/tmp/cvs-serv17731 > > >>>>>>> > > >>>>>>> Modified Files: > > >>>>>>> SplashOutputDev.cc > > >>>>>>> Log Message: > > >>>>>>> > > >>>>>>> added null checks (otherwise crash on win32 test pdf file) > > >>>>>> > > >>>>>>This doesn't sound like a fix to me. How this can possibly > > >>>>happen? Is it > > >>>>>>just a bad code usage or a bug in the code? > > >>>>>> > > >>>>>>I don't like the change because it papers over maybe a real bug. > > >>>>>>Please revert the change and file a bug with the test pdf attached. > > >>>>>>Maybe we will find out that the crash is not windows specific. > > >>>>> > > >>>>>Please revert this change. > > >>>> > > >>>>i do not see why, we have already discussed it. > > >>> > > >>>What about arguments mentioned in this email. You have never answered > > >>>any of of them. > > >>>The only thing that came from you is that you don't have a document > > >>>which caused the issue. > > >>>Are we talking about the same thing? It doesn't seem so... > > >> > > >>arguments that we will rather wait for a crash than use this code? > > >>sorry, do not accept. > > > > > >How do you want to debug that without any crash? > > >Please note that we have never seen any, except for the one which you > > >cannot reproduce anymore and which could be a side effect of the const > > >rework which has been fixed recently. > > > > i canot find the file now, i am testing pdf on 100> files sometimes > > so sadly but it can happen. > > > > > > > >This your change has my NACK and I will revert it if you do not do it. > > >Sorry this is just not acceptable. We don't have to do hacks like that. > > >Feel free to assign any bug report caused by this SEGV. > > > > again, you prefer crash and debugging over simply not null checks. > > This is how the code is written and your change didn't check other > places where the transpGroupStack is used. So this is a plain hack - > sorry that I repeat myself. > > > something imho crazy; however, xpdf is your code so you rule here. > > You apparently don't understand the code path or how it could happen. > I don't know that as well but I rather want to be conservative than add > something that none of us understands. If we happen to crash there then > we can debug it. Otherwise there might be some hidden error poping out > somewhere else - nothing I would like to debug... > So I don't see anything crazy about that. Reverted. -- Michal Hocko |
From: Michal H. <ms...@gm...> - 2010-04-27 21:19:39
|
On Tue, Apr 27, 2010 at 08:07:35PM +0300, Jozef Misutka wrote: > On Tue, 27 Apr 2010 15:52:35 +0300, Michal Hocko <ms...@gm...> 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 > >><ms...@gm...> 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 [...] " 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 |
From: Michal H. <ms...@gm...> - 2010-04-28 11:34:07
|
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 <ms...@gm...> 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 > > >><ms...@gm...> 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 > > 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 > Pdf...@li... > https://lists.sourceforge.net/lists/listinfo/pdfedit-devel -- Michal Hocko |