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 logname var instead of the inline string "opw" everywhere #78

Merged
merged 2 commits into from
Dec 15, 2021

Conversation

JeroenDM
Copy link
Owner

@JeroenDM JeroenDM commented Nov 7, 2021

First merge #76 and rebase.

@JeroenDM JeroenDM mentioned this pull request Nov 7, 2021
4 tasks
@JeroenDM JeroenDM closed this Dec 10, 2021
@JeroenDM JeroenDM reopened this Dec 10, 2021
@JeroenDM
Copy link
Owner Author

I also tested this locally to circumvent CI not fixed yet in this branch.

@JeroenDM
Copy link
Owner Author

(Looking forward to the day we can compile with c++20 and make LOGNAME and std::string again, but that's just nitpicking I guess :) )

@simonschmeisser
Copy link
Collaborator

I had a quick glance at MoveIt PlanningScene and there it is const std::string LOGNAME, is there a real difference between either of them?

@JeroenDM
Copy link
Owner Author

I had a quick glance at MoveIt PlanningScene and there it is const std::string LOGNAME, is there a real difference between either of them?

Not really, I once got it as a review comment to use constexpr and therefore updated the docs:
moveit/moveit.ros.org#474.

But in hindsight it's a clear example of me taking nitpicking too far... :) I guess in general C++ programmers have a tendency to over optimize.

@simonschmeisser
Copy link
Collaborator

I guess in general C++ programmers have a tendency to over optimize.

Agreed. Seriously, I don't actually want to know the difference and I hope the compiler does it's job and the end result is just the same ...

@JeroenDM JeroenDM merged commit 59f5d57 into melodic-devel Dec 15, 2021
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