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

[Red Hat Bugzilla]

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

Bug ID: 1367598
           Summary: Review Request: gap-pkg-guava - Computing with
                    error-correcting codes
           Product: Fedora
           Version: rawhide
         Component: Package Review
          Severity: medium
          Priority: medium
          Assignee: [e-mail]
          Reporter: [e-mail]
        QA Contact: [e-mail]
                CC: [e-mail]

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-1.fc26.sr...
Fedora Account System Username: jjames
Description: GUAVA is a package that implements coding theory algorithms in
GAP.
Codes can be created and manipulated and information about codes can be
calculated.

GUAVA consists of various files written in the GAP language, and an
external program from J. S. Leon for dealing with automorphism groups of
codes and isomorphism testing functions.  Several algorithms that need
the speed are integrated in the GAP kernel.

The functions within GUAVA can be divided into four categories:
- Construction of codes.  GUAVA can construct non-linear, linear and
  cyclic codes over an arbitrary finite field.  Examples are
  HadamardCode, ReedMullerCode, BestKnownLinearCode, QRCode and
  GoppaCode.
- Manipulation of codes.  These functions allow the user to transform
  one code into another or to construct a new code from two codes.
  Examples are PuncturedCode, DualCode, DirectProductCode and UUVCode.
- Computation of information about codes.  This information is stored in
  the code record.  Examples are MinimumDistance, OuterDistribution,
  IsSelfDualCode and AutomorphismGroup.
- Generation of bounds on linear codes.  The table by Brouwer and
  Verhoeff (as it existed in the mid-1990s) is incorporated into GUAVA.
  For example, BoundsMinimumDistance.

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

[Red Hat Bugzilla]

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

Paulo Andrade  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |paulo.cesar.pereira.de.andr
                   |                            |[e-mail]
           Assignee|[e-mail]    |paulo.cesar.pereira.de.andr
                   |                            |[e-mail]
              Flags|                            |fedora-review?

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

[Red Hat 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 = 2000000000ul - (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.

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

[Red Hat 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_Req...
https://lists.fedoraproject.org/archives/list/[e-mail]/...
> 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 = 2000000000ul - (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.

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

[Red Hat 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 = (2000000000ul - (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.

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

[Red Hat 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.sr...

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

[Red Hat 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=b941a86552fa...
(I just fedpkg co'd parallel and it is indeed broken).

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

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

[Red Hat 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

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

[Red Hat 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

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

[Red Hat Bugzilla]

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

Fedora Update System  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |MODIFIED

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

[Red Hat 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

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

[Red Hat 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

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

[Red Hat 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

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

[Red Hat 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.

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

[Red Hat 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

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

[Red Hat 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.