Bug 1367598

  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.

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/devel@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 = 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.

  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.

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

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!

Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/gap-pkg-guava

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

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

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

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

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.

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.