Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/17400 )
Change subject: [util] Add a function to fetch non-default flags as a map ...................................................................... Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/17400/2/src/kudu/util/flags.cc File src/kudu/util/flags.cc: http://gerrit.cloudera.org:8080/#/c/17400/2/src/kudu/util/flags.cc@594 PS2, Line 594: // Instead of "flags_by_name.emplace(flag.name, std::move(flag))" using : // following approach as order of evaluation of parameters could be different : // leading to unexpected value in "flag.name". > How about using EmplaceOrUpdate() from gutil/map-util.h? Same below. Thanks for that post. However it's not related to the problem I'm trying to fix. See the explanation above. IIUC EmplaceOrUpdate()/insert_or_assign() is susceptible to the same issue of order of evaluation of function parameters because of use of std::move(). The way I've implemented is one way, other way I could think about is explicitly using a copy of the flag.name and then using any of the new EmplaceOrUpdate()/insert_or_assign(). const auto flag_name = flag.name; EmplaceOrUpdate(&flags_by_name, flag_name, std::move(flag)); http://gerrit.cloudera.org:8080/#/c/17400/2/src/kudu/util/flags.cc@596 PS2, Line 596: // leading to unexpected value in "flag.name". > flags_by_name.emplace(flag.name, std::move(flag)) Above approach was used previously L590. In the line above, it's not guaranteed that flag.name will be evaluated before std::move(flag) as it's dependent on the compiler. If std::move(flag) is evaluated first then the value in flag.name could be empty/garbage. That's what I meant by "order of evaluation of parameters". I remember this being the case in pre-C++11 world and from the first paragraph still looks to be the case (I haven't dived into the details but the solution is simple and consequences are dire so why risk it). https://en.cppreference.com/w/cpp/language/eval_order -- To view, visit http://gerrit.cloudera.org:8080/17400 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7e7c9fc4de9be05110dba5ca02f48d46b6b474b7 Gerrit-Change-Number: 17400 Gerrit-PatchSet: 2 Gerrit-Owner: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Comment-Date: Wed, 05 May 2021 17:16:11 +0000 Gerrit-HasComments: Yes
