Convert variables to structured bindings #13018
Conversation
tygyh
force-pushed
the
Use-structured-bindings
branch
from
9e9e980
to
6fe41e5
Compare
| @@ -319,8 +318,8 @@ TodoList ComputeActionsToDo(Manifest this_manifest, Manifest next_manifest) | |||
| void CleanUpTempDir(const std::string& temp_dir, const TodoList& todo) | |||
| { | |||
| // This is best-effort cleanup, we ignore most errors. | |||
| for (const auto& download : todo.to_download) | |||
| File::Delete(temp_dir + DIR_SEP + HexEncode(download.hash.data(), download.hash.size())); | |||
| for (const auto& [_filename, hash] : todo.to_download) | |||
There was a problem hiding this comment.
Even though this is unused, please don't use variables that start with an underscore. The C++ standard reserves many of those (with varying rules and scopes) and could break at any given moment.
I don't think this particular one is considered reserved, but it feels out-of-place to me.
There was a problem hiding this comment.
How should I mark unused variables? There are very many when using structured bindings.
There was a problem hiding this comment.
I'd just name them as you'd name a used one, plus potentially apply [[maybe_unused]] if it raises an unused variable warning (see the other remark.)
There was a problem hiding this comment.
There are no warnings raised, I just want it to be clear which parts are actually in use.
| const auto op = *op_it; | ||
| std::string build_info_path = | ||
| temp_dir + DIR_SEP + HexEncode(op.new_hash.data(), op.new_hash.size()); | ||
| const auto [_filename, old_hash, new_hash] = *op_it; |
There was a problem hiding this comment.
Same here.
One could even argue that this needs [[maybe_unused]] to prevent unused variable warnings. Or use std::tie which can use std::ignore. Or wait for C++26 with P2169R0 🤷
There was a problem hiding this comment.
Neither of the two suggestions work.
There was a problem hiding this comment.
Just leave them without the underscore then. I don't really see a need to distinguish them in naming just because they're (intentionally) unused or not.
Source/Core/WinUpdater/Platform.cpp
Outdated
| @@ -284,8 +283,8 @@ bool CheckBuildInfo(const BuildInfos& build_infos) | |||
| // The existing BuildInfo may have been modified. Be careful not to overly trust its contents! | |||
|
|
|||
| // If the binary requires more recent OS, inform the user. | |||
| auto os_check = OSVersionCheck(build_infos.next); | |||
| if (os_check.status == VersionCheckStatus::UpdateRequired) | |||
| auto [status, _current_version, _target_version] = OSVersionCheck(build_infos.next); | |||
tygyh
force-pushed
the
Use-structured-bindings
branch
from
6fe41e5
to
464c2ed
Compare
tygyh
force-pushed
the
Use-structured-bindings
branch
from
464c2ed
to
13e343c
Compare
|
Imo structured bindings mainly make sense when dealing with generic types like std::pair to assign meaningful names in place of first/second. I don't see a benefit when dealing with custom classes that already having meaningful names for data members. |
Most of the SBs are just those meaningful names without the class instances as prefixes, so the lines just become a little bit shorter. I'll try to use SBs only where I think they make the code easier to read instead of applying it everywhere. (Some SBs have 10+ members and only one is used) |
|
Shorter code doesn't necessarily mean more readable code. Or that the original wasn't readable in first place. The goal isn't to make things as short as possible, but as readable and maintainable as possible. |
I agree, but I don't think the class instance names add enough context to warrant being read most of the time. |
|
Consider this: If I change the declaration order of struct members for some reason, I now have to update each structured binding that unpacks this struct. And if the member types match, I could easily miss an instance without triggering an error! |
I didn't know that. Anything which can cause silent errors I agree should be used sparingly, if at all. |
No description provided.