gap-packages / guava

GAP package guava - computations relative to error-correcting codes
https://gap-packages.github.io/guava
Other
13 stars 7 forks source link

Fix priority computation in the C code (preserve behavior) #83

Closed fingolfin closed 2 years ago

fingolfin commented 2 years ago

(Alternative to PR #82)

This fixes another C compiler warning:

./src/ccent.c:89:37: warning: operator '<<' has lower precedence than '-'; '-' will be evaluated first [-Wshift-op-parentheses]
            priority = 2000000000ul - (unsigned long) MIN(cycleLen[pt],1000) << 20
                       ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ~~
./src/ccent.c:89:37: note: place parentheses around the '-' expression to silence this warning
            priority = 2000000000ul - (unsigned long) MIN(cycleLen[pt],1000) << 20
                       ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./src/ccent.c:90:24: warning: operator '<<' has lower precedence than '+'; '+' will be evaluated first [-Wshift-op-parentheses]
                       + cSize;
                       ^~~~~~~
./src/ccent.c:90:24: note: place parentheses around the '+' expression to silence this warning
                       + cSize;
                       ^
                              )

This patch adds parenthesis to silence the warnings, thus preserving the current behavior. Whether this is the correct way to fix the warning remains unclear...

codecov[bot] commented 2 years ago

Codecov Report

Merging #83 (1527cf2) into master (e1eb3d0) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #83   +/-   ##
=======================================
  Coverage   49.37%   49.37%           
=======================================
  Files          44       44           
  Lines       14668    14668           
  Branches      203      203           
=======================================
  Hits         7242     7242           
  Misses       7426     7426           
dimpase commented 2 years ago

IMHO #82 is more plausable