On Fri, Oct 16, 2015 at 8:34 PM, Ronald S. Bultje <rsbul...@gmail.com> wrote: > Hi, > > On Fri, Oct 16, 2015 at 8:18 PM, Ganesh Ajjanagadde <gajja...@mit.edu> > wrote: > >> On Fri, Oct 16, 2015 at 8:05 PM, Ronald S. Bultje <rsbul...@gmail.com> >> wrote: >> > Hi, >> > >> > On Fri, Oct 16, 2015 at 5:48 PM, Ganesh Ajjanagadde <gajja...@mit.edu> >> > wrote: >> > >> >> On Fri, Oct 16, 2015 at 5:45 PM, Hendrik Leppkes <h.lepp...@gmail.com> >> >> wrote: >> >> > On Fri, Oct 16, 2015 at 11:39 PM, Ganesh Ajjanagadde >> >> > <gajjanaga...@gmail.com> wrote: >> >> >> On Wed, Oct 14, 2015 at 10:05 PM, Ganesh Ajjanagadde >> >> >> <gajjanaga...@gmail.com> wrote: >> >> >>> This patch results in identical behavior of movenc, and suppresses >> >> -Wstrict-overflow >> >> >>> warnings observed in GCC 5.2: >> >> >>> >> >> >> http://fate.ffmpeg.org/log.cgi?time=20150926231053&log=compile&slot=x86_64-archlinux-gcc-threads-misc >> >> , >> >> >>> "warning: assuming signed overflow does not occur when assuming that >> >> (X - c) > X is always false [-Wstrict-overflow]" >> >> >>> I have manually checked that all usages are safe, and overflow >> >> possibility does >> >> >>> not exist with this expression rewrite. >> >> >>> >> >> >>> Some expressed concern over readability loss, hence a comment is >> added. >> >> >>> This is the simplest way to suppress this warning. >> >> >>> >> >> >>> Signed-off-by: Ganesh Ajjanagadde <gajjanaga...@gmail.com> >> >> >>> --- >> >> >>> libavformat/movenc.c | 4 +++- >> >> >>> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> >>> >> >> >>> diff --git a/libavformat/movenc.c b/libavformat/movenc.c >> >> >>> index 5115585..ff997f2 100644 >> >> >>> --- a/libavformat/movenc.c >> >> >>> +++ b/libavformat/movenc.c >> >> >>> @@ -854,7 +854,9 @@ static int get_cluster_duration(MOVTrack *track, >> >> int cluster_idx) >> >> >>> { >> >> >>> int64_t next_dts; >> >> >>> >> >> >>> - if (cluster_idx >= track->entry) >> >> >>> + /* GCC 5.2 wants to "optimize" cluster_idx >= track->entry to >> the >> >> below >> >> >>> + * expression. We actually mean cluster_idx >= track->entry. */ >> >> >>> + if (cluster_idx - track->entry >= 0) >> >> >>> return 0; >> >> >>> >> >> >>> if (cluster_idx + 1 == track->entry) >> >> >>> -- >> >> >>> 2.6.1 >> >> >>> >> >> >> >> >> >> ping, is this solution acceptable? Note that I will get rid of the >> >> >> extra * on the second line of the comment. >> >> > >> >> > Not a fan myself of any such hacks to get compilers to shut up. Is GCC >> >> > doing something clearly wrong here, and the false warning should be >> >> > reported? >> >> >> >> I gave an (extremely detailed) explanation of what was going on >> >> earlier in the thread: >> >> https://ffmpeg.org/pipermail/ffmpeg-devel/2015-October/180976.html. >> >> The punch line is that it is not clearly wrong, and alternatives for >> >> warning suppression are less clean. >> > >> > >> > And I responded in the same thread: >> > https://ffmpeg.org/pipermail/ffmpeg-devel/2015-October/180977.html >> > >> > I'm still not a fan of this either. I don't think clang should be doing >> > what it's doing here. >> >> Neither am I a fan of this whole business. I try to make do with what >> is feasible, clean, and easy to revert at a future stage. >> >> My report was with gcc, have not checked clang. Funny that both of >> them try to do this. >> >> What alternative do you propose? Just keep the warning as it is, and >> forget suppressing it? >> >> Also, I would like to point out the following. Just see the following >> lines in avcodec/aacdec_template.c: >> /* This assignment is to silence a GCC warning about the variable >> being used >> * uninitialized when in fact it always is. >> */ >> pulse.num_pulse = 0; >> >> I have tested, and confirmed that only compilers strictly prior to GCC >> 4.6 actually complain. I showed this to Michael, and he did not feel >> like removing this ridiculous bit of code. It was in light of this >> that I crafted a patch with a comment, and thought people would be >> fine with it. > > > Sometimes these things are just based on personal opinions. :( I personally > wouldn't mind removing the assignment if recent gcc versions don't complain > anymore.
I guess the reason why I am keen on the movenc suppression is that I highly value compiler warnings, and am trying to move FFmpeg towards a clean build (I have outlined my rationale before). I admit, initially I was very naive: in personal projects I always have -Werror, -pedantic, -Wall, -Wextra, -Wshadow and god knows what :). I wondered why FFmpeg is not as liberal and why warnings are not taken as seriously. -Warray-bounds (and the current false positives) made me realise that for large projects, it is very hard to have clean builds. Nevertheless, I am still triving towards that goal. Why? It lets us fruitfully use warnings to our advantage - currently they are drowned in a sea of noise and very few hence pay attention to them. Let us face it: The above movenc example is likely to become a dead link on the GCC bug tracker. Even more likely, GCC may claim that this is not really a GCC bug. To me, in such a situation the choice is clear - we suppress it. I took your point and placed a comment, so that people know what we want semantically. It is a trivial patch and may be easily reverted, why not just add it (with the comment) and remove as soon as GCC "fixes" it in some future release? > > Ronald > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
