On Thu, Jul 12, 2018 at 8:02 PM Pedro Alves <pal...@redhat.com> wrote: > > On 07/12/2018 05:17 PM, Richard Sandiford wrote: > > Pedro Alves <pal...@redhat.com> writes: > > >> (an > >>> alternative to pointers is to return a struct with the wide int result > >>> and the overflow flag), > >> > >> +1. I've been pushing GDB in that direction whenever possible. > > > > I agree that can sometimes be better. I guess it depends on the > > context. If a function returns a bool and some other data that has no > > obvious "failure" value, it can be easier to write chained conditions if > > the function returns the bool and provides the other data via an output > > parameter. > > I agree it depends on context, though your example sounds like a > case for std::optional<T>. (We have gdb::optional<T> in gdb, since > std::optional is C++17 and gdb requires C++11. LLVM has a similar > type, I believe.) > > > > >>> but ... > >>> > >>>> +int *elt = &array[i]; // OK > >>>> +int &elt = array[i]; // Please avoid > >>> > >>> ... this seems unnecessary. If the function is so long that the fact > >>> elt is a reference can easily get lost, the problem is the length of > >>> the function, not the use of a reference. > >>> > >> > >> +1000. This seems really unnecessary. References have the advantage > >> of implicit never-null semantics, for instance. > > > > The nonnullness is there either way in the above example though. > > > > I don't feel as strongly about the non-const-reference variables, but for: > > > > int &elt = array[i]; > > ... > > elt = 1; > > > > it can be easy to misread that "elt = 1" is changing more than just > > a local variable. > > I think that might be just the case for people who haven't used > references before, and once you get some exposure, the effect > goes away. See examples below.
Agreed. > > > > > I take Jonathan's point that it shouldn't be much of a problem if > > functions are a reasonable size, but we've not tended to be very > > good at keeping function sizes down. I guess part of the problem > > is that even functions that start off small tend to grow over time. > > > > We have been better at enforcing more specific rules like the ones > > in the patch. And that's easier to do because a patch either adds > > references or it doesn't. It's harder to force (or remember to force) > > someone to split up a function if they're adding 5 lines to one that is > > already 20 lines long. Then for the next person it's 25 lines long, etc. > > > > > Also, the "int *elt" approach copes with cases in which "elt" might > > have to be redirected later, so we're going to need to use it in some > > cases. > > Sure, if you need to reseat, certainly use a pointer. That actually > highlights an advantage of references -- the fact that you are sure > nothing could reseat it. With a local pointer, you need to be mindful > of that. "Does this loop change the pointer?" Etc. If the semantics > of the function call for having a variable that is not meant to be > reseated, why not allow expressing it with a reference. > "Write what you mean." Of course, just like all code, there's going > to be a judgment call. > > A place where references frequently appear is in C++11 range-for loops. > Like, random example from gdb's codebase: > > for (const symbol_search &p : symbols) > > GCC doesn't yet require C++11, but it should be a matter > of time, one would hope. > > Other places references appear often is in coming up with > an alias to avoid repeating long expressions, like > for example (again from gdb): > > auto &vec = objfile->per_bfd->demangled_hash_languages; > auto it = std::lower_bound (vec.begin (), vec.end (), > MSYMBOL_LANGUAGE (sym)); > if (it == vec.end () || *it != MSYMBOL_LANGUAGE (sym)) > vec.insert (it, MSYMBOL_LANGUAGE (sym)); > > I don't think the fact that "vec" is a reference here confuses > anyone. > > That was literally a random sample (grepped for "&[a-z]* = ") so > I ended up picking one with C++11 "auto", but here's another > random example, spelling out the type name, similarly using > a reference as a shorthand alias: > > osdata_item &item = osdata->items.back (); > > item.columns.emplace_back (std::move (data->property_name), > std::string (body_text)); > > > It's then a question of whether "int &elt" is useful enough that > > it's worth accepting it too, and having a mixture of styles in the codebase. > I don't see it as a mixture of styles, as I don't see > pointers and references the exact same thing, but rather > see references as another tool in the box. Agreed, agreed, agreed :). Seriously folks, I really think we should concentrate on cleaning up GCC, and making it more readable, not nitpicking over style issues. Look at our wide int implementation for instance-- it adheres perfectly to our coding guidelines, and yet it's a mess mess to read (*). (*) I'm not singling out wide int because Richard worked on it. I believe it was Zadeck after all. It's just one of many examples (ahem, vec.h, tree-vrp.c, etc etc).
