mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alex Rukletsov <a...@mesosphere.io>
Subject Re: Shadowing variable names
Date Mon, 02 Feb 2015 10:19:05 GMT
MPark,

thank you for bringing this up! My 3ยข on this issue:

1. IMO the main point of leading/trailing underscores, camelCase,
type_Prefixes is to increase the Hamming distance between logically
different instances and therefore facilitate code understanding and reduce
bugs.

2. If these additions do not help understand the code or slow you down when
scanning the sources, they are undesirable. In other words, the API should
be clean. Hence in this case

class Foo {
public:
  Foo(int _var) : var(_var) {}  // Comment: Unnecessary leading underscore.
  int var;
};

I would prefer not bothering a Foo user with shadowing problems we have in
Foo implementation.

3. Regardless of what solution we all agree upon, let's document it in our
style guide.

My personal preference: google-like naming
<https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Variable_Names>.
Having trailing underscores for class members eliminates most cases of
shadowing, is not exposed in API, speeds-up understanding non-const class
methods.

Thanks,
Alex

On Sun, Feb 1, 2015 at 3:48 AM, Michael Park <mcypark@gmail.com> wrote:

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

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message