Turning on -Wshadow is a no-brainer to me. It absolutely helps keep a codebase readable and maintainable.
On Mon, Feb 2, 2015 at 2:19 AM, Alex Rukletsov <[email protected]> wrote: > 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. > +1, but this is a bigger change across the code-base. I really enjoy seeing which variables are private members and which are local/public at a glance. > > Thanks, > Alex > > On Sun, Feb 1, 2015 at 3:48 AM, Michael Park <[email protected]> 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. > > > -- Dominic Hamon | @mrdo | Twitter *There are no bad ideas; only good ideas that go horribly wrong.*
