----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71520/#review217879 -----------------------------------------------------------
Bad review! Reviews applied: [71520, 71519] Error: Circular dependency detected for review 71519.Please fix the 'depends_on' field. - Mesos Reviewbot On Sept. 19, 2019, 3:44 p.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71520/ > ----------------------------------------------------------- > > (Updated Sept. 19, 2019, 3:44 p.m.) > > > Review request for mesos, Benno Evers and Benjamin Mahler. > > > Bugs: MESOS-9948 > https://issues.apache.org/jira/browse/MESOS-9948 > > > Repository: mesos > > > Description > ------- > > This patch fixes some inefficient access patterns around `hashmap::get`. > Since this function returns an `Option` it can be used as a shorthand > for a `contains` check and subsequent creation of a value (`Option` > always contains a value). It was never not intended and is inefficient > as `contains` itself (e.g., via `hashmap::get::isSome`), and for cases > where only access to parts of the value in the `hashmap` is required > (e.g., to access a member of an optional value). In both these cases we > neither want nor need to create a temporary, and should instead either > just use `contains`, or access the value with `hashmap::at` after a > `contains` check; otherwise we might spend a lot of time creating > unnecessary temporary values. > > This patch fixes some easily identifiable cases found from skimming the > result of the following clang-query command: > > match cxxMemberCallExpr( > on(hasType(cxxRecordDecl(hasName("hashmap")))), > unless( > hasParent(cxxBindTemporaryExpr( > hasParent(materializeTemporaryExpr( > hasParent(exprWithCleanups( > hasParent(varDecl())))))))), > callee(cxxMethodDecl(hasName("get")))) > > This most probably misses a lot of cases. Given how easy it is to misuse > `hashmap::get` we should consider whether it makes sense to get rid of > it completely in lieu of an inlined form trading some additional lookups > for temporary avoidance, > > Option<X> x = map.contains(k) ? Some(map.at(k)) : Option<X>::none(); > > > Diffs > ----- > > 3rdparty/libprocess/src/metrics/metrics.cpp > 623d44adbe838f995ddbe89ee26f5bcc9c600be5 > > > Diff: https://reviews.apache.org/r/71520/diff/2/ > > > Testing > ------- > > `make check` > > > Thanks, > > Benjamin Bannier > >
