-
Notifications
You must be signed in to change notification settings - Fork 68
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 structured bindings #2322
Use structured bindings #2322
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR could/should also fix all usage of std::tie
.
ksp_plugin/interface.cpp
Outdated
@@ -180,8 +180,8 @@ serialization::OblateBody::Geopotential MakeGeopotential( | |||
} | |||
|
|||
serialization::OblateBody::Geopotential result; | |||
for (auto const& pair : rows) { | |||
*result.add_row() = pair.second; | |||
for (auto const& [degree, row] : rows) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering if we should write:
for (auto const& [_, row] : rows) {
in the case where one of the elements is not used. That would maybe make the code more readable (otherwise I am left wondering where degree
is used).
Iteration on maps
This changes most instances of
to
The exceptions are
Principia/physics/solar_system_body.hpp
Lines 636 to 638 in a8cfa72
where the name
degrees_of_freedom
is taken,and the public functions of the journal proto processor:
Principia/tools/journal_proto_processor.cpp
Lines 112 to 188 in a8cfa72
In both cases, switching to structured bindings felt clumsier than the existing code. It is noteworthy that both are examples of the same pattern, turning the values of a map into a vector; perhaps we could write an utility function for that.
_
for unused keysWhere the key is unused, this pull request uses
In the future, we will probably want to introduce an iterator adapter
ValuesIn
and change these usages toIterator adapters would also make it possible to turn usages of
Vessel::ForAllParts
into actual loops.insert
andemplace
use
bool const inserted = ⟨container⟩.insert(...).second
if only the boolean is used, reusing an existingbool inserted
if there are scoping issues; use structured bindings if both are used (where previously eitherstd::tie
or references into the pair would be used).One usage of
std::tie
remains, which is a proper tie of pre-existing variables:Principia/ksp_plugin/plugin.cpp
Lines 365 to 386 in c5a4878
Note that in the future, we should probably change
LOG_IF
in our glog fork to useif
rather than?:
, so that it is possible to narrowly scopeinserted
, thus