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 all uninitialized variable warnings #134

Merged
merged 1 commit into from
Aug 24, 2017

Conversation

jschueller
Copy link
Collaborator

@jschueller jschueller commented Aug 23, 2017

Every variables that yields a -Wmaybe-uninitialized gcc warning are set to 0

Fixes #133

Every variables that yields a -Wmaybe-uninitialized gcc warning is set to 0

Fixes stevengj#133
@ocommowi
Copy link
Contributor

I tested (cf #36 comments) on my crashes and it does solve the problem on the few examples I have. Thanks again !

Looking at the code again I still have the feeling that variables used as indexes should be initialised to 1 rather than 0 for fortran codes converted to C (see in bobyqa.c, altmov_ function for example, all table pointers have a -- applied to them at the start of the function and thus index 0 should not be accessed). That's my humble opinion though, again I get lost easily with these indexing conventions problems

@jschueller
Copy link
Collaborator Author

jschueller commented Aug 24, 2017

I dont know. I tried looking at the original code and it's really undefined behavior: IBDSAV could be not initialized and the code has <0 and >0 branches. I guess if you initialize it to 0 no branch is choosed but if 1 then the >0 is choosed. This is really an error in the original code. At least initializing it will make it deterministic.

@ocommowi
Copy link
Contributor

Yes I agree too that the original code has an error. For me, the >0 and <0 branches of the code are there to handle possible negative indexes and make them positive again when searching in the tables, but 0 is not handled. That's also why I preferred to initialise at 1 but 0 is working for me, and as you said it will at least make it deterministic so it is good to do this initialization anyway.

@jschueller jschueller merged commit c43afa0 into stevengj:master Aug 24, 2017
@jschueller jschueller deleted the uninitialized branch August 24, 2017 13:40
@jschueller
Copy link
Collaborator Author

that's really unclear, I propose we stick to 0 everywhere.

@stevengj
Copy link
Owner

stevengj commented Aug 24, 2017

On the one hand, 0 is probably a good default when in doubt; this code was translated from Fortran (via f2c), and if I remember correctly there were ancient versions of Fortran in which variables were implicitly initialized to zero. (The original author of this code, Powell, definitely harks back to those days… it's very much Fortran-66 style spaghetti.)

On the other hand, you have to be careful about masking one bug with another bug by initializing to an arbitrary value... at least tools like valgrind can catch uses of uninitialized variables, but they can't catch incorrect initializations.

@stevengj
Copy link
Owner

Looking at the code, it seems right to be to use ibdsav = 0, causing it to skip the final xnew[±ibdsav] = ... assignment. The preceding loop is supposed to identify the dimension where predsq is biggest, and uses ibdsav to push xnew in that dimension to the boundaries of the trust region. But if for some strange reason predsq is not biggest along any direction (maybe because it is always zero??? not sure why this would happen), then it seems right not to do any further modification of xnew.

@jschueller
Copy link
Collaborator Author

ok thank you for peaking in

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.

3 participants