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