[Bug 1367598] Review Request: gap-pkg-guava - Computing with error-correcting codes

[bugzilla]

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



--- Comment #12 from Fedora Update System  ---
gap-pkg-guava-3.13.1-2.fc24 has been pushed to the Fedora 24 stable repository.
If problems still persist, please make note of it in this bug report.

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

[Bug 1367598] Review Request: gap-pkg-guava - Computing with error-correcting codes

[bugzilla]

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



--- Comment #11 from Fedora Update System  ---
gap-pkg-guava-3.13.1-2.fc25 has been pushed to the Fedora 25 stable repository.
If problems still persist, please make note of it in this bug report.

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

[Bug 1367598] Review Request: gap-pkg-guava - Computing with error-correcting codes

[bugzilla]

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

Fedora Update System  changed:

   What|Removed |Added

 Status|ON_QA   |CLOSED
 Resolution|--- |ERRATA
Last Closed||2016-09-04 13:39:56



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

[Bug 1367598] Review Request: gap-pkg-guava - Computing with error-correcting codes

[bugzilla]

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



--- Comment #10 from Fedora Update System  ---
gap-pkg-guava-3.13.1-2.fc24 has been pushed to the Fedora 24 testing
repository. If problems still persist, please make note of it in this bug
report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here:
https://bodhi.fedoraproject.org/updates/FEDORA-2016-2547786f05

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

[Bug 1367598] Review Request: gap-pkg-guava - Computing with error-correcting codes

[bugzilla]

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

Fedora Update System  changed:

   What|Removed |Added

 Status|MODIFIED|ON_QA



--- Comment #9 from Fedora Update System  ---
gap-pkg-guava-3.13.1-2.fc25 has been pushed to the Fedora 25 testing
repository. If problems still persist, please make note of it in this bug
report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here:
https://bodhi.fedoraproject.org/updates/FEDORA-2016-bcd5fe40a8

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

[Bug 1367598] Review Request: gap-pkg-guava - Computing with error-correcting codes

[bugzilla]

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

Fedora Update System  changed:

   What|Removed |Added

 Status|NEW |MODIFIED



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

[Bug 1367598] Review Request: gap-pkg-guava - Computing with error-correcting codes

[bugzilla]

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



--- Comment #8 from Fedora Update System  ---
gap-pkg-guava-3.13.1-2.fc24 has been submitted as an update to Fedora 24.
https://bodhi.fedoraproject.org/updates/FEDORA-2016-2547786f05

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

[Bug 1367598] Review Request: gap-pkg-guava - Computing with error-correcting codes

[bugzilla]

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



--- Comment #7 from Fedora Update System  ---
gap-pkg-guava-3.13.1-2.fc25 has been submitted as an update to Fedora 25.
https://bodhi.fedoraproject.org/updates/FEDORA-2016-bcd5fe40a8

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

[Bug 1367598] Review Request: gap-pkg-guava - Computing with error-correcting codes

[bugzilla]

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



--- Comment #6 from Jon Ciesla  ---
Package request has been approved:
https://admin.fedoraproject.org/pkgdb/package/rpms/gap-pkg-guava

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

[Bug 1367598] Review Request: gap-pkg-guava - Computing with error-correcting codes

[bugzilla]

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

Paulo Andrade  changed:

   What|Removed |Added

  Flags|fedora-review?  |fedora-review+



--- Comment #5 from Paulo Andrade  ---
Looks like the parallel command is broken in rawhide:
$ head -1 /usr/bin/parallel 
##!/usr/bin/perl
note the two '#'.
Fixing it manually and building the package I do not see
any other issue.
You might want to either check what is broken with the
parallel package, or not use it... See the sed command in
http://pkgs.fedoraproject.org/cgit/rpms/parallel.git/commit/?id=b941a86552fa42ca018a58bffc3ba35c428b0d07
(I just fedpkg co'd parallel and it is indeed broken).

  Otherwise, the package is now approved. All the issues
have been fixed!

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

[Bug 1367598] Review Request: gap-pkg-guava - Computing with error-correcting codes

[bugzilla]

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



--- Comment #4 from Jerry James  ---
I added those parentheses.  I also noticed that guava has a software popcount
implementation, so I added another patch to use a CPU popcount instruction
instead if one is available.  New URLs:

Spec URL: https://jjames.fedorapeople.org/gap-pkg-guava/gap-pkg-guava.spec
SRPM URL:
https://jjames.fedorapeople.org/gap-pkg-guava/gap-pkg-guava-3.13.1-2.fc26.src.rpm

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

[Bug 1367598] Review Request: gap-pkg-guava - Computing with error-correcting codes

[bugzilla]

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



--- Comment #3 from Paulo Andrade  ---
  I think the last one is likely a bug, because the "+ cSize"
is added in the next line, but (adding parenthesis,) what is
happening is:

priority = (20ul - (unsigned long)MIN(cycleLen[pt],1000)) << (20 +
cSize);

  So, it might have two bugs.
  The smaller the value of cycleLen[pt] and cSize, the more
likely the code does what is intended, as it appears to be
some kind of heuristic priority choosing.

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

[Bug 1367598] Review Request: gap-pkg-guava - Computing with error-correcting codes

[bugzilla]

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



--- Comment #2 from Jerry James  ---
Thanks for the review, Paulo.

(In reply to Paulo Andrade from comment #1)
>   It is not required to have:
> BuildRequires: gcc

Actually, it is now.  See:
https://fedoraproject.org/wiki/Packaging:C_and_C%2B%2B#BuildRequires_and_Requires
https://lists.fedoraproject.org/archives/list/de...@lists.fedoraproject.org/message/XGXCCAJNEOIEN3KK6TN2657LSIGZGB3N/

>   This looks suspecting, as it means it will loop only once:
> ./src/randschr.c:238:10: warning: this 'if' clause does not guard...
> [-Wmisleading-indentation]

Good catch!  I will add braces to the warning patch to fix that.

>   || and && have different precedence, so this code is also suspecting:
> ./src/randschr.c:391:43: warning: suggest parentheses around '&&' within
> '||' [-Wparentheses]
>lowestOrder == curOrder && levelLowestOrder > finalLevel)
> ) {
>^~~~

That one I think is okay.  It appears to me that the && really should be within
||, so it is parsed in the correct order.  But ... it's easy to patch, so I
will add parentheses there.

>   Another case of suspicious code:
> ./src/ccent.c: In function 'nextBasePointEltCent':
> ./src/ccent.c:90:24: warning: suggest parentheses around '+' inside '<<'
> [-Wparentheses]
>  priority = 20ul - (unsigned long)
> MIN(cycleLen[pt],1000) << 20
> 
> ~~
> + cSize;
> ^~~

This one worries me.  I'm not sure if the + is supposed to be inside or outside
of the <<, so I am reluctant to touch it.  I think I'd better check with
upstream before I do anything here, unless you think you can see where the
parentheses should go.

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

[Bug 1367598] Review Request: gap-pkg-guava - Computing with error-correcting codes

[bugzilla]

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



--- Comment #1 from Paulo Andrade  ---
  It is not required to have:
BuildRequires: gcc

---
  This looks suspecting, as it means it will loop only once:
./src/randschr.c:238:10: warning: this 'if' clause does not guard...
[-Wmisleading-indentation]
  if ( hOrder % primeList[i] == 0 )
  ^~
./src/randschr.c:240:13: note: ...this statement, but the latter is
misleadingly indented as if it is guarded by the 'if'
 break;
 ^
code is:
  for ( i = 0 ; primeList[i] != 0 ; ++i ) {
 if ( hOrder % primeList[i] == 0 )
raisePermToPower( h, hOrder / primeList[i]);
break;
  }
and the data is:
guava-3.13.1/src/leon/src/primes.c:Unsigned primeList[] =
{2,3,5,7,9,11,13,17,19,23,29,31,37,41,43,47,53,59, ... 0};

---
  || and && have different precedence, so this code is also suspecting:
./src/randschr.c:391:43: warning: suggest parentheses around '&&' within '||'
[-Wparentheses]
   lowestOrder == curOrder && levelLowestOrder > finalLevel) )
{
   ^~~~
code is:
if ( nonInvolRejectCount > 0 &&
(...||   (lowestOrder < (curOrder = permOrder(h)) ||
 ...&&...)   lowestOrder == curOrder && levelLowestOrder > finalLevel) ) {

---
  Another case of suspicious code:
./src/ccent.c: In function 'nextBasePointEltCent':
./src/ccent.c:90:24: warning: suggest parentheses around '+' inside '<<'
[-Wparentheses]
 priority = 20ul - (unsigned long) MIN(cycleLen[pt],1000)
<< 20
   
 ~~
+ cSize;
^~~

---
  The package appears in good shape, but while I checked all other
compiler warnings, and they appear to just warn about improper
indentation or somewhat uncommon coding practive, the above warnings
may be valid, so, I would like some comment about it.

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

[Bug 1367598] Review Request: gap-pkg-guava - Computing with error-correcting codes

[bugzilla]

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

Paulo Andrade  changed:

   What|Removed |Added

 CC||paulo.cesar.pereira.de.andr
   ||a...@gmail.com
   Assignee|nob...@fedoraproject.org|paulo.cesar.pereira.de.andr
   ||a...@gmail.com
  Flags||fedora-review?



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