Use structured bindings #2322

eggrobin · 2019-09-02T00:22:40Z

Iteration on maps

This changes most instances of

for (auto const& pair : ⟨map⟩)

to

for (auto const& [key, value] : ⟨map⟩)

The exceptions are

Principia/physics/solar_system_body.hpp

Lines 636 to 638 in a8cfa72

    
           for (auto const& pair : name_to_degrees_of_freedom) { 
        
             degrees_of_freedom.push_back(pair.second); 
        
           }

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

    
           std::vector<std::string> 
        
           JournalProtoProcessor::GetCsInterfaceMethodDeclarations() const { 
        
             std::vector<std::string> result; 
        
             for (auto const& pair : cs_interface_method_declaration_) { 
        
               result.push_back(pair.second); 
        
             } 
        
             return result; 
        
           } 
        
           std::vector<std::string> 
        
           JournalProtoProcessor::GetCsInterfaceTypeDeclarations() const { 
        
             std::vector<std::string> result; 
        
             for (auto const& pair : cs_interface_type_declaration_) { 
        
               result.push_back(pair.second); 
        
             } 
        
             return result; 
        
           } 
        
           std::vector<std::string> 
        
           JournalProtoProcessor::GetCxxInterfaceMethodDeclarations() const { 
        
             std::vector<std::string> result; 
        
             for (auto const& pair : cxx_interface_method_declaration_) { 
        
               result.push_back(pair.second); 
        
             } 
        
             return result; 
        
           } 
        
           std::vector<std::string> 
        
           JournalProtoProcessor::GetCxxInterfaceTypeDeclarations() const { 
        
             std::vector<std::string> result; 
        
             for (auto const& pair : cxx_interface_type_declaration_) { 
        
               result.push_back(pair.second); 
        
             } 
        
             return result; 
        
           } 
        
           std::vector<std::string> 
        
           JournalProtoProcessor::GetCxxInterchangeImplementations() const { 
        
             std::vector<std::string> result; 
        
             result.push_back("namespace {\n\n"); 
        
             for (auto const& pair : cxx_deserialize_definition_) { 
        
               result.push_back(pair.second); 
        
             } 
        
             for (auto const& pair : cxx_serialize_definition_) { 
        
               result.push_back(pair.second); 
        
             } 
        
             result.push_back("}  // namespace\n\n"); 
        
             return result; 
        
           } 
        
           std::vector<std::string> 
        
           JournalProtoProcessor::GetCxxMethodImplementations() const { 
        
             std::vector<std::string> result; 
        
             for (auto const& pair : cxx_functions_implementation_) { 
        
               result.push_back(pair.second); 
        
             } 
        
             return result; 
        
           } 
        
           std::vector<std::string> JournalProtoProcessor::GetCxxMethodTypes() const { 
        
             std::vector<std::string> result; 
        
             for (auto const& pair : cxx_toplevel_type_declaration_) { 
        
               result.push_back(pair.second); 
        
             } 
        
             return result; 
        
           } 
        
           std::vector<std::string> JournalProtoProcessor::GetCxxPlayStatements() const { 
        
             std::vector<std::string> result; 
        
             result.push_back("{\n  bool ran = false;\n"); 
        
             for (auto const& pair : cxx_play_statement_) { 
        
               result.push_back(pair.second); 
        
             } 
        
             result.push_back("  CHECK(ran) << method_in->DebugString() << \"\\n\"\n" 
        
                              "             << method_out_return->DebugString();\n}\n"); 
        
             return result; 
        
           }

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 keys

Where the key is unused, this pull request uses

for (auto const& [_, value] : ⟨map⟩)

In the future, we will probably want to introduce an iterator adapter ValuesIn and change these usages to

for (auto const& value : ValuesIn(⟨map⟩))

Iterator adapters would also make it possible to turn usages of Vessel::ForAllParts into actual loops.

`insert` and `emplace`

use bool const inserted = ⟨container⟩.insert(...).second if only the boolean is used, reusing an existing bool inserted if there are scoping issues; use structured bindings if both are used (where previously either std::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

    
           void Plugin::InsertOrKeepVessel(GUID const& vessel_guid, 
        
                                           std::string const& vessel_name, 
        
                                           Index const parent_index, 
        
                                           bool const loaded, 
        
                                           bool& inserted) { 
        
             CHECK(!initializing_); 
        
             not_null<Celestial const*> parent = 
        
                 FindOrDie(celestials_, parent_index).get(); 
        
             auto it = vessels_.find(vessel_guid); 
        
             if (it == vessels_.end()) { 
        
               std::tie(it, inserted) = 
        
                   vessels_.emplace( 
        
                       vessel_guid, 
        
                       make_not_null_unique<Vessel>(vessel_guid, 
        
                                                    vessel_name, 
        
                                                    parent, 
        
                                                    ephemeris_.get(), 
        
                                                    DefaultPredictionParameters())); 
        
             } else { 
        
               inserted = false; 
        
             } 
        
             not_null<Vessel*> const vessel = it->second.get();

Note that in the future, we should probably change LOG_IF in our glog fork to use if rather than ?:, so that it is possible to narrowly scope inserted, thus

CHECK(bool const inserted = container.insert(...).second; inserted) << ...;

pleroy

This PR could/should also fix all usage of std::tie.

pleroy · 2019-09-02T04:52:04Z

ksp_plugin/interface.cpp

@@ -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) {


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).

ksp_plugin/plugin.cpp

eggrobin added 9 commits September 1, 2019 23:55

use structured bindings in some obvious and easy places

8877a6c

vessels

0c621ca

more vessels

359ff15

children of Ilúvatar

7e6d869

initial states

80391b2

why is ->* even a thing?

f6e25be

forgot a reference

be8b073

[it, inserted]

a0e8d76

const

0a6da74

pleroy reviewed Sep 2, 2019

View reviewed changes

ksp_plugin/plugin.cpp Show resolved Hide resolved

eggrobin added 6 commits September 2, 2019 21:05

uniformize insert/emplace and turn ties into structured bindings

e4ee351

second

afdc4a7

comment lifetime

b68ea32

inserted

2bf18c2

unused

1237f62

_

34c5f8f

eggrobin changed the title ~~Use structured bindings when iterating on maps~~ Use structured bindings Sep 2, 2019

pleroy added the LGTM label Sep 2, 2019

eggrobin merged commit 31f2c60 into mockingbirdnest:master Sep 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use structured bindings #2322

Use structured bindings #2322

eggrobin commented Sep 2, 2019 •

edited

pleroy left a comment

pleroy Sep 2, 2019

	for (auto const& pair : name_to_degrees_of_freedom) {
	degrees_of_freedom.push_back(pair.second);
	}

	std::vector<std::string>
	JournalProtoProcessor::GetCsInterfaceMethodDeclarations() const {
	std::vector<std::string> result;
	for (auto const& pair : cs_interface_method_declaration_) {
	result.push_back(pair.second);
	}
	return result;
	}

	std::vector<std::string>
	JournalProtoProcessor::GetCsInterfaceTypeDeclarations() const {
	std::vector<std::string> result;
	for (auto const& pair : cs_interface_type_declaration_) {
	result.push_back(pair.second);
	}
	return result;
	}

	std::vector<std::string>
	JournalProtoProcessor::GetCxxInterfaceMethodDeclarations() const {
	std::vector<std::string> result;
	for (auto const& pair : cxx_interface_method_declaration_) {
	result.push_back(pair.second);
	}
	return result;
	}

	std::vector<std::string>
	JournalProtoProcessor::GetCxxInterfaceTypeDeclarations() const {
	std::vector<std::string> result;
	for (auto const& pair : cxx_interface_type_declaration_) {
	result.push_back(pair.second);
	}
	return result;
	}

	std::vector<std::string>
	JournalProtoProcessor::GetCxxInterchangeImplementations() const {
	std::vector<std::string> result;
	result.push_back("namespace {\n\n");
	for (auto const& pair : cxx_deserialize_definition_) {
	result.push_back(pair.second);
	}
	for (auto const& pair : cxx_serialize_definition_) {
	result.push_back(pair.second);
	}
	result.push_back("} // namespace\n\n");
	return result;
	}

	std::vector<std::string>
	JournalProtoProcessor::GetCxxMethodImplementations() const {
	std::vector<std::string> result;
	for (auto const& pair : cxx_functions_implementation_) {
	result.push_back(pair.second);
	}
	return result;
	}

	std::vector<std::string> JournalProtoProcessor::GetCxxMethodTypes() const {
	std::vector<std::string> result;
	for (auto const& pair : cxx_toplevel_type_declaration_) {
	result.push_back(pair.second);
	}
	return result;
	}

	std::vector<std::string> JournalProtoProcessor::GetCxxPlayStatements() const {
	std::vector<std::string> result;
	result.push_back("{\n bool ran = false;\n");
	for (auto const& pair : cxx_play_statement_) {
	result.push_back(pair.second);
	}
	result.push_back(" CHECK(ran) << method_in->DebugString() << \"\\n\"\n"
	" << method_out_return->DebugString();\n}\n");
	return result;
	}

	void Plugin::InsertOrKeepVessel(GUID const& vessel_guid,
	std::string const& vessel_name,
	Index const parent_index,
	bool const loaded,
	bool& inserted) {
	CHECK(!initializing_);
	not_null<Celestial const*> parent =
	FindOrDie(celestials_, parent_index).get();
	auto it = vessels_.find(vessel_guid);
	if (it == vessels_.end()) {
	std::tie(it, inserted) =
	vessels_.emplace(
	vessel_guid,
	make_not_null_unique<Vessel>(vessel_guid,
	vessel_name,
	parent,
	ephemeris_.get(),
	DefaultPredictionParameters()));
	} else {
	inserted = false;
	}
	not_null<Vessel*> const vessel = it->second.get();

Use structured bindings #2322

Use structured bindings #2322

Conversation

eggrobin commented Sep 2, 2019 • edited

Iteration on maps

_ for unused keys

insert and emplace

pleroy left a comment

Choose a reason for hiding this comment

pleroy Sep 2, 2019

Choose a reason for hiding this comment

eggrobin commented Sep 2, 2019 •

edited

`_` for unused keys

`insert` and `emplace`