Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix compiler warnings about uninitialised variables and issue #15 #58

Merged
merged 5 commits into from
Aug 25, 2020
Merged

Fix compiler warnings about uninitialised variables and issue #15 #58

merged 5 commits into from
Aug 25, 2020

Conversation

robertgj
Copy link
Contributor

  1. Fix compiler warnings about uninitialised variables.

  2. Fix issue results are not apparently deterministic? #15 "results are not apparently deterministic?" in blkchol2.c:
    --- sedumi/blkchol2.c 2020-08-15 23:10:22.858943847 +1000
    +++ SeDuMi_1_3/blkchol2.c 2020-08-15 23:09:49.000000000 +1000
    @@ -113,7 +113,7 @@
    ------------------------------------------------------- /
    xkk = x[inz];
    if(xkk > lb[k]){ /
    now xkk > 0 */

  •  if(xkk < ub){
    
  •  if((m>1) && (xkk < ub)){
       ubk = maxabs(x+inz+1,m-1) / maxu;
       if(xkk < ubk){
    
  1. Apply fix to sedumi.m recommended in SparsePOP readme.txt (the second is already in):

NOTE4: SeDuMi in Github (2014-08-14) has two bugs to use it. We list
two modifications of SeDuMi:
(1) Remove % in the head of the 792nd line in sedumi.m
(2) Replace the 77th line in eigK.m by
lab(li+1:li+nl) = x(xi+1:xi+nl);

dpr1fact.c Outdated
@@ -365,7 +365,7 @@ char dodpr1fact(double *beta, mwIndex *perm, double *d, double t, const double *
------------------------------------------------------------ */
else{
psqrdep = 0.0;
for(i = 0; dep[i] < m; i++)
for(i = 0, j = 0; dep[i] < m; i++)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this change. Please set j in a separate line before for clarity.

install_sedumi.m Outdated
Comment on lines 180 to 181
flags{end+1} = '-Wno-unused-variable';
flags{end+1} = '-Wno-unused-but-set-variable';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Obviously not supported by clang (macOS) https://travis-ci.org/github/sqlp/sedumi/jobs/718183038

Is it too difficult to remove the unused variables as well? Then those flags are not needed.

sedumi.m Outdated Show resolved Hide resolved
@siko1056
Copy link
Member

Thank you for your effort @robertgj . I reviewed your changes and suggest two changes.

@robertgj
Copy link
Contributor Author

Respond to siko1056 requested changes

@siko1056
Copy link
Member

siko1056 commented Aug 20, 2020

Thank you again @robertgj for your changes. The reason for the macOS buildbot to fail is OpenMathLib/OpenBLAS#2783 and is no blocker for the merge.

Just one question before I merge: Where is the macro MEX_DEBUG documented for Matlab (I know it is given in Octave)? I think your intention is to avoid the unused variable warning in case mxAssert() statements are not compiled, right?

@siko1056 siko1056 self-assigned this Aug 20, 2020
@robertgj
Copy link
Contributor Author

I'm afraid I don't have access to MATLAB. Add -DMEX_DEBUG in the mex template to enable mxAssert checks when compiling MATLAB with install_sedumi.m.

@siko1056
Copy link
Member

Having a closer look at it, I am convinced introducing the definition -DMEX_DEBUG is the wrong way to go. -Wall -Wextra are removed from install_sedumi() anyway, thus why complicating things now?

Can you strip your second commit (3d0e453) and just care about my two assertions for the first commit (313c7d2)?

  1. Fix compiler warnings about uninitialised variables and results are not apparently deterministic? #15 (313c7d2)
  2. Fix unused variable warnings when compiling SeDuMi (3d0e453)

Sorry, I don't want to delay things unnecessary, but quick fixes now are painful to deal with later.

@robertgj
Copy link
Contributor Author

robertgj commented Aug 20, 2020

Revert previous change to robertgj/sedumi as requested by siko1056.

I am not sure what is happening with examples/README.md and examples/test_sedumi.m. They are the same in my clones of robertgj/sedumi and sqlp/sedumi.

  1. blkchol2.c : do not call BLAS FORTRAN IDAMAX with an array of length 1
  2. sedumi.m : add 'Octave:singular-matrix' warning id
  3. maxstep.m, trydif.m, widelen.m : suppress octave-6 warning about implicit all
  4. dpr1fact.c, bwblkslv.c, spscale.c fwblkslv.c urotorder.c,: suppress compiler
    warnings about unused and uninitialised variables
  5. psdframeit.c, sqrtinv.c, symfctmex.c, vecsym.c: suppress compiler warnings
    about unused variables

Copy link
Member

@siko1056 siko1056 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All changes look good, I care about the files I changed in the "examples" directory, if things go wrong.

@siko1056 siko1056 merged commit 2af87c0 into sqlp:master Aug 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants