[Bug 1269539] Review Request: mozjs38 - JavaScript interpreter and libraries

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



https://bugzilla.redhat.com/show_bug.cgi?id=1269539



--- Comment #3 from Marek Skalický <mskalick@xxxxxxxxxx> ---
Thank you for the review Paulo!

> 
>   Looking at build log, most warnings appear ok, but this
> one might have issues:
> 
> In file included from
> /builddir/build/BUILD/mozjs-38.0.0/js/src/js/src/Unified_cpp_js_src2.cpp:65:
> 0:
> /builddir/build/BUILD/mozjs-38.0.0/js/src/irregexp/RegExpEngine.cpp: In
> member function 'virtual bool js::irregexp::TextNode::FillInBMInfo(int, int,
> js::irregexp::BoyerMooreLookahead*, bool)':
> /builddir/build/BUILD/mozjs-38.0.0/js/src/irregexp/RegExpEngine.cpp:4605:48:
> warning: array subscript is above array bounds [-Warray-bounds]
>                          bm->Set(offset, chars[j]);
> 
> looking at the code it appears to be a false positive, likely
> because g++ did not understand that it should always return
> a value that is in bounds. But I may be mistaken, please
> comment.

It looks good to me.
j is always in the array bounds.

> 
> ---8<---
> mozjs38.src:18: W: mixed-use-of-spaces-and-tabs (spaces: line 18, tab: line
> 1)
> 
> Looks like you added a missing build requires from mozjs31
> spec, but did not match spaces vs tabs :)

Fixed:-)

> 
> ---8<---
> In js.pc I see:
> Version: 38.3.0esrpre
> 
> Looks like there is some confusion. But this is minor,
> just a comment. Should be expected from a .rc0.
> 

I've changed it to 38.2.1.rc0

> ---8<---
> I believe is is not strictly an update do mozjs31, so you may
> modify the changelog to not say so. It should only have conflicts
> with mozjs31-devel and mozjs38-devel, otherwise, runtime will
> not conflict.

OK, you are right. Fixed.

> 
> ---8<---
> Please remove the bundled mozjs-38.0.0/modules/zlib/
> directory before starting build (mozjs31 does not have
> a modules subdir). It should not use it due to --with-system-zlib,
> but just to be sure.

OK.

> 
> ---8<---
> There are some new files under apache license, so, it should
> be required to add "ASL 2.0" to the License tag.
> In mozjs31 asmjs was simpler, in the js/src/jit
> directory, and under MPL 2.0 license. Now it is
> under js/src/asmjs/AsmJS and with apache license.


OK. Also added AFL -
js/src/jit-test/tests/sunspider/check-string-unpack-code.js

-- 
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
https://admin.fedoraproject.org/mailman/listinfo/package-review




[Index of Archives]     [Fedora Legacy]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]