Hello,
TL;DR: There has been a few review comments suggesting to shadow variable
names in order to avoid the leading/trailing underscore in the name. In
general, this only leads to stupid bugs that waste developers' time, we can
eliminate these bugs and also the need to reevaluate if the names of
variables need to be changed when adding new code. I'd like to at least
discourage this practice from our codebase and in the best case scenario,
turn *-Wshadow* on and make a pass over the codebase to remove shadowing
variable names.
class Foo {
public:
// Prefer '_var' over 'var' here.
Foo(int _var) : var(_var) {}
int var;
};
NOTE: The leading underscore is a non-standard naming scheme but that's a
separate issue being tracked at
https://issues.apache.org/jira/browse/MESOS-1046
This discussion arose from https://reviews.apache.org/r/30395/#comment115306,
the following is the typical situation where the discussion arises.
Review Request:
class Foo {
public:
Foo(int _var) : var(_var) {} // Comment: Unnecessary leading underscore.
int var;
};
In this case, that is true. However, I think shadowing variable names
should be avoided in general, especially between local variables (including
function parameters) and member variables because the member variable
declarations are typically lexically not in scope.
Here are a few examples of how this can lead to problems in the future.
1)
Present:
> 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;
> };
Future:
> struct Obj
> {
> Obj(Foo &&foo)
> : foo(std::move(foo)) {
> // New author: "ok, let's do some stuff with member 'foo'!"
> // oops... this is likely to segfault.
> std::cout << foo << std::endl;
> }
> Foo foo;
> };
2)
Present:
> // 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' ...
> }
Future:
> 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;
> }
>From Cody Maloney:
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.
In short, I think we should opt to stay away from running into issues like
this by not shadowing variable names. 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.
Thanks,
MPark.
