El Dilluns, 19 de novembre de 2012, a les 08:16:59, Adam Reichold va escriure: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > Hello Albert, > > I am sorry about that. It seems like my editor messed up with the > spaces since it seems to apply using "--ignore-space-change". I tried > to reformat the patch using said option, hopefully this helps. (It > still has some whitespace warnings but no more errors.)
Can you have a look at https://bugs.freedesktop.org/attachment.cgi?id=18330 The dash line that divides the the green rectangular area changes from grayish to black when applying your patch. Cheers, Albert > > Regards, Adam. > > On 18.11.2012 23:01, Albert Astals Cid wrote: > > El Dimecres, 14 de novembre de 2012, a les 18:48:26, Adam Reichold > > va escriure: Hello again, > > > > The attached patch should rework the function copy constructors. > > > >> Doesn't seem to apply on current master, can you please rebase > >> it and make sure it applies? > >> > >> Cheers, Albert > > > > Best regards, Adam. > > > > On 14.11.2012 12:58, Albert Astals Cid wrote: > >>>> El Dimecres, 14 de novembre de 2012, a les 07:28:21, Adam > >>>> Reichold va escriure: Hello Albert, > >>>> > >>>> Before I try the other classes, could you have a look at the > >>>> > >>>> attached patch for SampledFunction and tell if this is what > >>>> > >>>> you are looking for? > >>>> > >>>> Would something like memcpy(sampleSize, func->sampleSize, > >>>> funcMaxInputs * sizeof(int)); be preferred to the plain > >>>> for-loops? > >>>> > >>>>> Yes. it's what the other functions use, right? > >>>> > >>>> Regards, Adam. > >>>> > >>>> P.S.: By the way, is there a reason to use the signature > >>>> Function(const Function*) for the "what would be a copy > >>>> constructor" instead of "Function(const Function&)"? > >>>> > >>>>> No clue, ask the guy that wrote them (hint, he's not > >>>>> around) > >>>>> > >>>>> Cheers, Albert > >>>> > >>>> On 13.11.2012 23:28, Albert Astals Cid wrote: > >>>>>>> El Dilluns, 27 d'agost de 2012, a les 22:31:13, Albert > >>>>>>> Astals Cid > >>>>>>> > >>>>>>> va escriure: > >>>>>>>> El Dilluns, 27 d'agost de 2012, a les 08:29:40, > >>>>>>>> Thomas Freitag va > >>>>>>>> > >>>>>>>> escriure: > >>>>>>>>> On 27.08.2012 00:56, Albert Astals Cid wrote: > >>>>>>>>>> El Diumenge, 26 d'agost de 2012, a les 15:48:37, > >>>>>>>>>> He Liu va > >>>>>>>>>> > >>>>>>>>>> escriure: > >>>>>>>>>>>>> 5. vtable pointer will be overwritten > >>>>>>>>>>>>> Function.cc:422:10: warning: destination > >>>>>>>>>>>>> for this 'memcpy' call is a pointer to > >>>>>>>>>>>>> dynamic class 'SampledFunction'; vtable > >>>>>>>>>>>>> pointer will be overwritten > >>>>>>>>>>>>> [-Wdynamic-class-memaccess] > >>>>>>>>>>>>> > >>>>>>>>>>>>> memcpy(this, func, > >>>>>>>>>>>>> sizeof(SampledFunction)); ~~~~~~ ^ > >>>>>>>>>>>>> > >>>>>>>>>>>>> Function.cc:422:10: note: explicitly cast > >>>>>>>>>>>>> the pointer to silence this warning > >>>>>>>>>>>>> > >>>>>>>>>>>>> At least categrory 5. sound serious to me, > >>>>>>>>>>>>> I would never have copied instances of C++ > >>>>>>>>>>>>> objects in that way, because it depends on > >>>>>>>>>>>>> the compiler and the class if this causes > >>>>>>>>>>>>> problems on runtime, s. i.e. > >>>>>>>>>>>>> http://weseetips.com/tag/afx_zero_init_object/, > > Note this is memset-ing to 0, not memcpy-ing a > > >>>>>>>>>>>> class to itself. To be honest i agree > >>>>>>>>>>>> memcpy'in a SampledFunction to a > >>>>>>>>>>>> SampledFunction is ugly, but i fail to see > >>>>>>>>>>>> why it would not work. > >>>>>>>>> > >>>>>>>>> It works, at least with the actual used compilers. > >>>>>>>>> But it works only, because the allocated members of > >>>>>>>>> SampledFunction are overwriten after doing the > >>>>>>>>> memcpy. And this behaviour makes it just more ugly > >>>>>>>>> in my eyes. And this is the same with the other > >>>>>>>>> memcpy's in ExponentialFunction, StitchingFunction > >>>>>>>>> and PostScriptFunction. And it will work till such > >>>>>>>>> time as everybody who changes the class will not > >>>>>>>>> forget to do it in the same way. So if You are not > >>>>>>>>> willing to change it (or let somebody else change > >>>>>>>>> it, I know, never change running code), we could > >>>>>>>>> use the hint: explicitly cast the pointer to > >>>>>>>>> silence this warning. > >>>>>>>> > >>>>>>>> I'm fine with someone fixing the code. Less warnings > >>>>>>>> is always good, but for master, there's no need to > >>>>>>>> introduce a pontential breaker just because clang > >>>>>>>> complains. > >>>>>>> > >>>>>>> So at the end noone volunteered to re-rewite those > >>>>>>> functions not to use memcpy over classes with > >>>>>>> virtuals? > >>>>>>> > >>>>>>> Cheers, Albert > >>>>>>> > >>>>>>>> Cheers, Albert > >>>>>>>> > >>>>>>>>> Cheers, Thomas > >>>>>>>>> > >>>>>>>>>>> Hi Albert, > >>>>>>>>>>> > >>>>>>>>>>> :-) > >>>>>>>>>>> > >>>>>>>>>>> A pointer of type SampleFunction* could be > >>>>>>>>>>> pointing to an instance of a SampleFunction > >>>>>>>>>>> sub-class, which has different vtable > >>>>>>>>>>> contents. > >>>>>>>>>> > >>>>>>>>>> No, it could not, SampledFunction does not have > >>>>>>>>>> any childs and the function doing that memcopy > >>>>>>>>>> is private anyway. > >>>>>>>>>> > >>>>>>>>>>> As a result, one could construct a > >>>>>>>>>>> SampleFunction with > >>>>>>>>>>> SampleFunction(SampleFunction *) using a > >>>>>>>>>>> pointer to a sub-class instance, and overwrite > >>>>>>>>>>> the SampleFunction's vtable address with the > >>>>>>>>>>> sub-class's vtable address. > >>>>>>>>>>> > >>>>>>>>>>> I am not sure if it will lead to any > >>>>>>>>>>> bugs/vulnerabilities in this case, but it is > >>>>>>>>>>> not safe practice in general. > >>>>>>>>>> > >>>>>>>>>> Sure, i never said it was. I'm just saying i > >>>>>>>>>> don't see why it would not work in our case. > >>>>>>>>>> > >>>>>>>>>>> Since the vtable structure depends on how the > >>>>>>>>>>> compiler is implemented, memcpy or memset on > >>>>>>>>>>> object pointers will generally lead to > >>>>>>>>>>> undefined behaviors. > >>>>>>>>>> > >>>>>>>>>> I'm far from a compiler expert, but one would > >>>>>>>>>> hope that for a given class the compiler stores > >>>>>>>>>> always the "stuff" in the same order in memory, > >>>>>>>>>> so again, i fail to see why this should fail in > >>>>>>>>>> our case. > >>>>>>>>>> > >>>>>>>>>> Cheers, > >>>>>>>>>> > >>>>>>>>>> Albert > >>>>>>>>>> > >>>>>>>>>>> Thanks. > >>>>>>>>>>> _______________________________________________ > > poppler mailing list [email protected] > > >>>>>>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler > > > > _______________________________________________ poppler > > > >>>>>>>>>> mailing list [email protected] > >>>>>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler > > . > > >>>>>>>>> _______________________________________________ > >>>>>>>>> poppler mailing list [email protected] > >>>>>>>>> > >>>>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler > > _______________________________________________ poppler > > >>>>>>>> mailing list [email protected] > >>>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler > > _______________________________________________ poppler > > >>>>>>> mailing list [email protected] > >>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler > >>>> > >>>> _______________________________________________ poppler > >>>> mailing list [email protected] > >>>> http://lists.freedesktop.org/mailman/listinfo/poppler > > > > _______________________________________________ poppler mailing > > list [email protected] > > http://lists.freedesktop.org/mailman/listinfo/poppler > > -----BEGIN PGP SIGNATURE----- > Version: GnuPG v2.0.19 (GNU/Linux) > > iQEcBAEBAgAGBQJQqdzrAAoJEPSSjE3STU34RisH/iqlAtA62bwUMNG9Ym6iRQhk > 7RXIqGED7gC7PNMp8gi/dGBH+ziRjVtQfag9oyq4L0QgqEkEHa4hH55WVSnaScik > 8qZrRkh9w8XNcG7ROURRbWniKYM0cj8Ao24ps6nJr6GODAS82LpWycFUgz+W4v9J > lA0jvkzhpJoS700L5rwUkj5xh+2X98Qy556yt3Hew1T7M1pCOAEWGc794k56/d4q > je/zec2xL2oMGP8jrEr01GDqshg+z/+dAqrkg+yJ6M3CzpCvA3iB/kJEQZAMFT1g > Jy6o7tn41WTk2FYlWmgPxna6bA+GvR4JT8dxrA47zOcmqTVoOtOfpa0o9+Wfvss= > =4ghL > -----END PGP SIGNATURE----- _______________________________________________ poppler mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/poppler
