fix: clang warnings. #508
fix: clang warnings. #508
Conversation
Signed-off-by: Roman Gershman <[email protected]>
| @@ -428,7 +428,7 @@ error_code Replica::InitiateDflySync() { | |||
| auto partition = Partition(num_df_flows_); | |||
| shard_set->pool()->AwaitFiberOnAll([&](unsigned index, auto*) { | |||
| for (auto id : partition[index]) { | |||
| if (ec = shard_flows_[id]->StartFullSyncFlow(&sb)) | |||
| if ((ec = shard_flows_[id]->StartFullSyncFlow(&sb))) | |||
There was a problem hiding this comment.
maybe the new still "if" with "if (ec = shard_flows_[id]->StartFullSyncFlow(&sb); ec) {..} would be more readable? right now this look a little bit like a classic C bug where you're assign and then git the side effect from assignment.
| @@ -871,7 +871,7 @@ error_code ServerFamily::DoSave(bool new_version, Transaction* trans, string* er | |||
| const auto scripts = script_mgr_->GetLuaScripts(); | |||
| auto& summary_snapshot = snapshots[shard_set->size()]; | |||
| summary_snapshot.reset(new RdbSnapshot(fq_threadpool_.get())); | |||
| if (ec = DoPartialSave(filename, path, start, scripts, summary_snapshot.get(), nullptr)) { | |||
| if ((ec = DoPartialSave(filename, path, start, scripts, summary_snapshot.get(), nullptr))) { | |||
There was a problem hiding this comment.
Same as the previous comment - I think that it would look more like the intent is to assign then check. assign with side effect is the classic that you;re not sure if this is a bug or you're using the side effect
| @@ -880,7 +880,7 @@ error_code ServerFamily::DoSave(bool new_version, Transaction* trans, string* er | |||
| auto cb = [&](Transaction* t, EngineShard* shard) { | |||
| auto& snapshot = snapshots[shard->shard_id()]; | |||
| snapshot.reset(new RdbSnapshot(fq_threadpool_.get())); | |||
| if (ec = DoPartialSave(filename, path, start, {}, snapshot.get(), shard)) { | |||
| if ((ec = DoPartialSave(filename, path, start, {}, snapshot.get(), shard))) { | |||
|
For some reason I thought the new If-with-initializer is c++20, so I avoided it. We should use it of course. Note that the code is even worse than you think it is 😁 AggregateValue's assignment operator returns the value of the argument. So it equivalent to the following. This is because we need to know if DoPartialSave started something or not, and we care less about the global error, but need to assign it afterwards. if (auto local_ec = DoPartialSave; local_ec) {
ec = local_ec
} |
|
looks weird. Frankly, I prefer avoiding using assignments in |
|
I can fix the AggregateValue-if issue separately |
|
I will submit it anyway to save the hop. Feel free to change when you touch that code :) |
Signed-off-by: Roman Gershman [email protected]