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 #140

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix compiler warnings #140

wants to merge 1 commit into from

Conversation

chris0x44
Copy link

Builds silently on MSVC with /W4

ATTENTION: Please take a close look at the lambda in optimizer.h:928
The parameter was overshadowing the capture which is interpreted in variable ways on different compilers. I made a guess on the intended behavior which might be very wrong.

@sudz123
Copy link
Owner

sudz123 commented Oct 26, 2018

The grammar corrections are appreciated, but we are trying to be consistent with the variable naming and are following the book for it, please do not change it.

@chris0x44
Copy link
Author

Just to be sure. You are referring to all changes in variable names or just the two for-loops, both using i as a counter?
Because using x in the lambda in MultiplierConstrained will produce inconsistent behavior depending on compiler ( see https://stackoverflow.com/questions/42088015/lambda-capture-and-parameter-with-same-name-who-shadows-the-other-clang-vs-g ).

ATTENTION: Please take a close look at the lambda in optimizer.h:928
The parameter was overshadowing the capture which is interpreted in variable ways on different compilers. I made a guess on the intended behavior which might be very wrong.
@sudz123
Copy link
Owner

sudz123 commented Oct 27, 2018

I was referring to the for loops, we are trying to keep the 'i' consistent.
Also, we are capturing the variable named x for use in the lambda function. I did not understand the details of the link you gave, I might if I dig deeper(which I will), but what I would like to know is why are you calling obj_func with value and GetMultiplierPenalty with x. They need to be called with the same variable, the x which was captured from the main function.

@chris0x44
Copy link
Author

OK, so the lambda should look as follows?

std::function<double (Eigen::VectorXd)> func = [R, obj_func, x, ineq_const, eq_const, sigma, tau] (Eigen::VectorXd value)->double { 
               return obj_func(x) + R * GetMultiplierPenalty(ineq_const, eq_const, x, tau, sigma); };

In that case I am confused as to why the lambda takes a parameter for the call if it doesn't use it.

@sudz123
Copy link
Owner

sudz123 commented Oct 30, 2018

Yes, that is how it should used. But I too am confused regarding your question. Which parameter are you referring to, because every parameter being captured is used inside the lambda function I think.

@chris0x44
Copy link
Author

Maybe we have a misunderstanding here. All values captured by the lambda are used, that I agree with. The thing is that the way the std::function object is defined, it would have the following function prototype (captures of the lambda are not relevant here):

double MyFuncName( Eigen::VectorXd value );

So the function, when it is invoked, needs to be passed an Eigen::VectorXd object. Since we are not using this passed parameter in the function, I was wondering why it is needed in the first place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants