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

Use Eigen expressions more effectively and kill & in code. #917

Merged
merged 2 commits into from
Nov 6, 2021

Conversation

dellaert
Copy link
Member

@dellaert dellaert commented Nov 6, 2021

Quick edit on @gchenfc 's recent PR #433
Be careful with having references for local variables. That's for arguments, not things on the stack. Also, though counter-intuitive I think doing transpose() twice here is actually more efficient as it will not actually store the transpose internally, which you forced. Read about Eigen expressions and tell me if I'm wrong :-)

@dellaert dellaert requested a review from gchenfc November 6, 2021 17:48
Copy link
Member

@gchenfc gchenfc left a comment

Choose a reason for hiding this comment

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

Yep, great on both accounts! Optional commentary on why I had these mistakes:

  1. const XXX &var = func() - I read on stackoverflow one time that this is the one special time it is legal to assign a const reference to a temporary or something along those lines, but after doing more reading now I think this was just factually wrong advice. Because I thought it was legal, I've been using this syntax because I was too lazy to check (or to remember) whether the function returns a const ref (e.g. for a member variable getter) or value. If the function returns a const ref, this will save a copy and if the function returns a value, I figured it doesn't make a difference so it's just easier to always assign the function return to const ref. But now I know it's illegal and will go back and fix code where I did this.

  2. Good catch! I just missed this.

@dellaert dellaert merged commit c0faaed into develop Nov 6, 2021
@dellaert dellaert deleted the fix/AdjointTranspose branch November 6, 2021 23:09
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