> On Jan. 29, 2015, 5:50 p.m., Alexander Rukletsov wrote: > > src/master/contender.cpp, line 82 > > <https://reviews.apache.org/r/30395/diff/1/?file=839643#file839643line82> > > > > You can safely omit trailing underscores. > > Michael Park wrote: > In this case, yes that is true. This topic has come up before and perhaps > it's better suited for the dev list so that we can get a community-wide > consensus on this. > > In general, I think we should try to avoid shadowing. *Especially* > between local variables (including function parameters) and member variables > because the member variable declarations are typically lexically not in scope. > > 1) > > ``` > struct Obj > { > // Original author: "Eh, we don't need to refer to the member 'foo' so > this is fine." > Obj(Foo &&foo) > : foo(std::move(foo)) {} > > Foo foo; > }; > ``` > > ``` > struct Obj > { > Obj(Foo &&foo) > : foo(std::move(foo)) { > // New author: "ok, let's do some stuff with member 'foo'!" > // Oops... this will segfault. > std::cout << foo << std::endl; > } > > Foo foo; > }; > ``` > > 2) > > ``` > // Original author: "There's a member 'foo' but I don't need it for this > function." > Obj::F(const Foo &foo) { > // Do stuff with local 'foo' ... > } > ``` > > ``` > Obj::F(const Foo &foo) { > // Do stuff with local 'foo' ... > // /* ... */ > // New author: (middle of debugging) "What state is (member) 'foo' > in...?" > // Local 'foo' gets printed instead, doesn't realize the problem until > they look above > // to find the shadowed variable and... fury. > std::cout << foo << std::endl; > } > ``` > > In short, I think we should opt to stay away from running into issues > like this by not shadowing member variables. > It keeps us away from running into stupid bugs that waste developer's > time, and also eliminates the need to reevaluate if the names of variables > need to be changed when adding new code. > > Cody Maloney wrote: > Some examples from the codebase of existing shadowing and problems which > could be encountered working / refactoring around them. > > stout/json.hpp: > ``` > inline Value convert(const picojson::value& value) > { > if (value.is<picojson::null>()) { > return Null(); > } else if (value.is<bool>()) { > return Boolean(value.get<bool>()); > } else if (value.is<picojson::value::object>()) { > Object object; > foreachpair (const std::string& name, > const picojson::value& value, > value.get<picojson::value::object>()) { > object.values[name] = convert(value); > } > return object; > } else if (value.is<picojson::value::array>()) { > Array array; > foreach (const picojson::value& value, > value.get<picojson::value::array>()) { > array.values.push_back(convert(value)); > } > return array; > } else if (value.is<double>()) { > return Number(value.get<double>()); > } else if (value.is<std::string>()) { > return String(value.get<std::string>()); > } > return Null(); > } > ``` > The foreachpair creates a second value which it uses in the assignment. > Yes, this works right, but when scanning over the code if I don't catch that > there was an additional definition embedded in the foreach... > > stout/flags.hpp: > ``` > template <typename Flags, typename T1, typename T2> > void FlagsBase::add( > T1 Flags::*t1, > const std::string& name, > const std::string& help, > const T2& t2) > { > Flags* flags = dynamic_cast<Flags*>(this); > if (flags == NULL) { > ABORT("Attempted to add flag '" + name + "' with incompatible type"); > } else { > flags->*t1 = t2; // Set the default. > } > > Flag flag; > flag.name = name; > flag.help = help; > flag.boolean = typeid(T1) == typeid(bool); > flag.loader = lambda::bind( > &MemberLoader<Flags, T1>::load, > lambda::_1, > t1, > lambda::function<Try<T1>(const std::string&)>( > lambda::bind(&parse<T1>, lambda::_1)), > name, > lambda::_2); > flag.stringify = lambda::bind( > &MemberStringifier<Flags, T1>, > lambda::_1, > t1); > > // Update the help string to include the default value. > flag.help += help.size() > 0 && help.find_last_of("\n\r") != > help.size() - 1 > ? " (default: " // On same line, add space. > : "(default: "; // On newline. > flag.help += stringify(t2); > flag.help += ")"; > > add(flag); > } > ``` > If the delegated add had been written inline (flags[flag.name] = flag), > then things wouldn't work right... > > > stout/os/killtre > ``` > inline Try<std::list<ProcessTree> > killtree( > pid_t pid, > int signal, > bool groups = false, > bool sessions = false) > { > ... > > while (!queue.empty()) { > pid_t pid = queue.front(); > queue.pop(); > > ... > > } > } > ``` > If I need to modify the code inside the while loop to do something like > exclude the original PID to resolve some sort of bug, I will have to rename a > lot of variables for a small change in functionality. Most likely I won't > notice that pid became even more local, and will first spend a while > debugging why the code "Reads" right but doesn't do what it reads as. > > Alexander Rukletsov wrote: > Ok, good points. Let's bring this up in the dev list, so we can have an > outcome out of this discussion that we document and start using it > consistently.
Feel free to drop the issue. - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30395/#review70231 ----------------------------------------------------------- On Jan. 29, 2015, 3:58 a.m., Cody Maloney wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30395/ > ----------------------------------------------------------- > > (Updated Jan. 29, 2015, 3:58 a.m.) > > > Review request for mesos and Benjamin Hindman. > > > Bugs: MESOS-1806 > https://issues.apache.org/jira/browse/MESOS-1806 > > > Repository: mesos > > > Description > ------- > > etcd master contender + detector > > > Diffs > ----- > > src/etcd/etcd.cpp PRE-CREATION > src/master/contender.hpp 76beb5f973ae02507849233b6d73c43293669489 > src/master/contender.cpp c1bf82b621d6b46afe001acafe9ee53336726406 > src/master/detector.hpp 2905e2b3536e14e9df3570da172603e6ed81aae1 > src/master/detector.cpp 700eb9dde8e71648bacc00a82766634f77cf2d15 > > Diff: https://reviews.apache.org/r/30395/diff/ > > > Testing > ------- > > > Thanks, > > Cody Maloney > >
