https://bugzilla.redhat.com/show_bug.cgi?id=1150441 --- Comment #13 from Ankur Sinha (FranciscoD) <sanjay.ankur@xxxxxxxxx> --- Hi Jerry, Thanks for the review. I've fixed what I could, and filed issues upstream about the rest: SPEC: https://ankursinha.fedorapeople.org/iv/iv.spec SRPM: https://ankursinha.fedorapeople.org/iv/iv-0-0.1.20191117git08c48bb.fc32.src.rpm (In reply to Jerry James from comment #11) > Issues > ============== > 1. Some of the compiler warnings are troubling. In particular: > a. -Wsequence-point: this indicates undefined behavior. In fact, this > warning seems to point to a typo in the code; idarrows.cpp line 514 is: > > _x[l] = _x[k] = _x[j] = tx1; _y[l] = _y[k] = _y[k] = ty1; > > Note the assignment of _y[k] to itself. By analogy with the _x array > settings, it seems probable that the second of those should be _y[j] > instead. ^ I filed an issue upstream and they fixed it as per your recommendation. > b. -Wstrict-aliasing: the compiler can produce code in this case that is > quite different from what you would expect, and can lead to crashes. I > have seen this happen in practice numerous times; this is not merely an > academic issue. One solution is to add -fno-strict-aliasing to the > build > flags, but that pessimizes the entire library. Another approach is to > use memcpy() to move the bits in question between entities of different > types. Still another approach is to store data in a union, so the same > bits are accessible as different types. Let me know if you would like > me > to examine the code and propose a solution. ^ Reported upstream https://github.com/neuronsimulator/iv/issues/14 > c. -Wchar-subscripts: the type "char" is problematic as an array index > type > because it is signed on some architectures and unsigned on others. The > code should be examined to ensure that, where it is signed, the > subscripts > only take on values in the range 0 to 127. > ^ Reported upstream https://github.com/neuronsimulator/iv/issues/15 > 2. This is in %files: > > # Nothing else owns it, obsolete? > %{_datadir}/app-defaults/ > > That's the wrong directory. It should be %{_datadir}/X11/app-defaults. > The > difficulty is that that directory is owned by libXt, but this package does > not Requires: libXt. Since that dependency is not generated > automatically, > you will probably have to add it manually. ^ Fixed. > > 3. Note the undefined-non-weak-symbol warnings from rpmlint. Those indicate > cases of underlinking. In particular, libIVhines.so.3.0.3 should have > been > linked with -lX11 and libUnidrawhines.so.3.0.3 should have been linked > with > -lIVhines. ^ Fixed this and sent a PR upstream. It was merged and I've used the newest git snapshot now. > 4. Not really an issue, just a tip. The "find" command takes a -delete flag, > so you could write the find line in %install less verbosely, like this: > > find $RPM_BUILD_ROOT -name '*.la' -delete ^ Fixed > > 5. Since the -devel subpackage Requires the main package, you do not need to > include the license file in -devel, but you may do so if you wish. Fixed. Upstream has said that they'll look into the compiler warnings. I'll also do so when I can find the time. Please feel free to help upstream if you have the time too :) https://github.com/neuronsimulator/iv/issues/13#issuecomment-554787755 Cheers, Ankur -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component _______________________________________________ package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx
