-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 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? 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&)"? 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 > -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.19 (GNU/Linux) iQEcBAEBAgAGBQJQozoFAAoJEPSSjE3STU34WSgIAKq8CWceSQbavyFRyos0QsXS cHTeh0Um9yfC4dIiaqa5ZwFOVH0OGkjWe02FtcygQGfNCgWeEHmod2ONFM3px3YK 3m4L8d3O/mxxtJ3bOq9u8XVFoZN2+JFElWL/ZKdoELGd+g8LnrZ2yWTqKo4ZRqMT yw9yfkjMI3gqryD/aYTF9vGIKbw10zCBH80myuINQbja+SoNbZF1qmQqyWd1cp6v qdyDHgXNSF4vlbrIa0E4ju/MMqtvVVwK42IHa8toP/h53CzP5DweZXR+ln5/lMoG SWJwDoQ731RY1U25f5xaom4ZwmQ0KO9lhmeYd0cvsyFDhQqg828NkN2pzUTvIGQ= =AleA -----END PGP SIGNATURE-----
>From 26fd36fa3d6da133a1880c2d61b936fff455159a Mon Sep 17 00:00:00 2001 From: Adam Reichold <[email protected]> Date: Wed, 14 Nov 2012 07:20:02 +0100 Subject: [PATCH] reworked copy constructor of Function and SampledFunction --- poppler/Function.cc | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++-- poppler/Function.h | 4 +++- 2 files changed, 53 insertions(+), 3 deletions(-) diff --git a/poppler/Function.cc b/poppler/Function.cc index d26aed8..4328834 100644 --- a/poppler/Function.cc +++ b/poppler/Function.cc @@ -178,6 +178,23 @@ GBool Function::init(Dict *dict) { return gFalse; } +Function::Function(const Function *func) { + m = func->m; + n = func->n; + + for (int i = 0; i < funcMaxInputs; ++i) { + domain[i][0] = func->domain[i][0]; + domain[i][1] = func->domain[i][1]; + } + + for (int i = 0; i < funcMaxOutputs; ++i) { + range[i][0] = func->range[i][0]; + range[i][1] = func->range[i][1]; + } + + hasRange = func->hasRange; +} + //------------------------------------------------------------------------ // IdentityFunction //------------------------------------------------------------------------ @@ -419,13 +436,44 @@ SampledFunction::~SampledFunction() { } } -SampledFunction::SampledFunction(SampledFunction *func) { - memcpy(this, func, sizeof(SampledFunction)); +SampledFunction::SampledFunction(const SampledFunction *func) : Function(func) { + // parameters + + for (int i = 0; i < funcMaxInputs; ++i) { + sampleSize[i] = func->sampleSize[i]; + + encode[i][0] = func->encode[i][0]; + encode[i][1] = func->encode[i][1]; + + decode[i][0] = func->decode[i][0]; + decode[i][1] = func->decode[i][1]; + + inputMul[i] = func->inputMul[i]; + } + + ok = func->ok; + + // samples + + nSamples = func->nSamples; + idxOffset = (int *)gmallocn(1 << m, sizeof(int)); memcpy(idxOffset, func->idxOffset, (1 << m) * (int)sizeof(int)); + samples = (double *)gmallocn(nSamples, sizeof(double)); memcpy(samples, func->samples, nSamples * sizeof(double)); + sBuf = (double *)gmallocn(1 << m, sizeof(double)); + + // buffers + + for (int i = 0; i < funcMaxOutputs; ++i) { + cacheIn[i] = func->cacheIn[i]; + } + + for (int i = 0; i < funcMaxOutputs; ++i) { + cacheOut[i] = func->cacheOut[i]; + } } void SampledFunction::transform(double *in, double *out) { diff --git a/poppler/Function.h b/poppler/Function.h index 25df133..23ea3ab 100644 --- a/poppler/Function.h +++ b/poppler/Function.h @@ -89,6 +89,8 @@ public: protected: static Function *parse(Object *funcObj, std::set<int> *usedParents); + + Function(const Function *func); int m, n; // size of input and output tuples double // min and max values for function domain @@ -140,7 +142,7 @@ public: private: - SampledFunction(SampledFunction *func); + SampledFunction(const SampledFunction *func); int // number of samples for each domain element sampleSize[funcMaxInputs]; -- 1.8.0
_______________________________________________ poppler mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/poppler
