On 12/15/2015 10:39 PM, Jean Delvare wrote: > I see two completely different things. > > The change I proposed is specific to one function, only changes const > vs non-const, and the object under discussion is being accessed for > reading only (thus the const pointer.) It would remove a memcpy but > does not introduce a direct copy (with =.)
See below. > The change you pointed us to is touching very generic functions, > handling by nature a very wide variety of pointer types for both > reading and writing. > > So just because the change you pointed us to may be good and desirable, > doesn't imply that the change I am proposing is bad and undesirable. > > Side note #1: if gcc really considers pointer types that only differ by > "const" as different when it comes to code optimization, it is > seriously broken IMHO. Unfortunately the man page is quite vague on the > topic ("unless the types are almost the same", can you be more vague > please.) The C spec does. GCC happens to handle it correctly. We support a whole lot more compilers than GCC. > Side note #2: if strict-aliasing optimizations can do so much damage > to your code that you end up doing > memcpy(arg, &(void *){ NULL }, sizeof(val)); > to set a pointer to NULL, maybe you should consider building with > -fno-strict-aliasing. Again, that is GCC-specific. My point is that if we *can* be correct, with regards to the spec, we should be. We don't gain anything, IMO, by removing this particular thing. > > Side note #3: I'm surprised this change was needed in the first place > as my understanding was that gcc would skip all strict-aliasing > optimizations for void pointers, which is what the affected functions > are handling. It's sad that the commit message is as vague as gcc's > manual page on the topic. If a change makes some code more spec compliant, with little to no downside, I fail to see why it shouldn't be incldued. Your patch here makes code *less* spec compliant for little to no gain. P.S. It actually looks like the original code is not entirely compliant, after discussing with some people Smarter Than Me: [17:06] <+courmisch> memory copying the pointer fixes the aliasing problem on pointer itself [17:06] <+courmisch> but I suspect it leaves an aliasing problem with the pointed data [17:07] <+courmisch> specifically, I am not sure the standard allows creating pointers to existing data out of thin air [17:07] <+courmisch> which memcpy() effectively does if it changes the pointer type So, as a I see it, this patch replaced one aliasing violation with two. If an argument can be made that there is a measurable speedup, and none of our supported compilers abuse the C spec, as compilers are wont to do, then perhaps it's worth revisiting, but otherwise, I'd prefer to NAK it. I'm obviously not The Final Word here, but this is my 2 cents. - Derek _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel