> On May 2, 2018, 5:49 p.m., James Peach wrote: > > I think the original author expected the `using > > mesos::v1::scheduler::Call;` declaration to cause `Call::SUBSCRIBE` to find > > the `v1` version. I'm fine with this change, especially if we are getting > > compiler warnings for it by default. It's not that obvious from code > > inspection why `scheduler::Call::SUBSCRIBE` ends up finding the right enum > > ... maybe there's a way to make it more obvious to the casual reader?
I am not sure we can make this more readable to the casual reader, I had to confirm with my editor that the correct types were found. I think the issue comes from us using using decls to liberally which in general can way to easily interfere in unexpected ways with ADL in a language as C++. In a world where we'd just use them to locally to trigger ADL this would be less of an issue :( - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66903/#review202282 ----------------------------------------------------------- On May 2, 2018, 11:36 a.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66903/ > ----------------------------------------------------------- > > (Updated May 2, 2018, 11:36 a.m.) > > > Review request for mesos and James Peach. > > > Bugs: MESOS-8865 > https://issues.apache.org/jira/browse/MESOS-8865 > > > Repository: mesos > > > Description > ------- > > Due to ADL the enum values found originally found in this function where > from the v0 namespace while the value we compared to was from the v1 > namespace. While this might work should the enum values in v0 and v1 map > to the same enum values, it does not seem to have been the intention and > is brittle. > > This patch uses slightly longer names to refer to the enum values which > do not map onto symbols found via ADL. > > > Diffs > ----- > > src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp > ea8d54ff198a5529d61a41bcb6e5806378761091 > > > Diff: https://reviews.apache.org/r/66903/diff/1/ > > > Testing > ------- > > `make check` with clang. While before clang emitted diagnostics for the lines > touched here, no such diagnostics are emitted anymore with this patch. > > > Thanks, > > Benjamin Bannier > >
