> 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.
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. - 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-git > > > 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 > >
