Re: Implement -Wimplicit-fallthrough (version 5)

[Martin Sebor <msebor at gmail dot com>]

> > I'm curious why this function takes the first argument by const
> > reference when the function above by const pointer.  Is there
> > a subtle difference between the functions that I'm not seeing?
> > For that matter, is there a convention in GCC?  (FWIW, my own
> > rule of thumb is to have a function take a large argument by
> > const reference when it must be non-null and by pointer
> > otherwise.)
>
> I admit I don't know what's better, I'd say the reference, but elsewhere in the
> codebase I see const *.  I changed case_label_p's vec argument to const * to be
> consistent, but I'm all ears if anyone wants to educate me.

There's been lots of debate about this in various C++ communities.
This may be a discussion better had outside of a patch review but
since you asked I'll answer here.

I don't think there is one right answer, but having a convention
in place would be helpful especially to those of us who are not
intimately familiar with the details of every API in the compiler.

One coding style recommends using const references and non-const
pointers (and not the other way around).  The rationale is to
make it possible to tell just by looking at a call to a function
whether it can change an argument.  For example, in:

  SomeType x = /* ... */;
  OtherType y = /* ... */;

  foo (x, &y);

the convention makes it clear that x will be unchanged by the call
to foo but y will most likely not.  This is true whether foo takes
x by value or by const reference.  The downside of this approach
is that the callee has to (or should) either assert the that the
pointer is non-null or handle the null, otherwise it can raises
the question whether it's intended to accept null pointers.

This style is what the Google Coding Style recommends:
https://google.github.io/styleguide/cppguide.html#Reference_Arguments

Marshall Cline's C++ FAQ LITE considers this style old school and
recommends to use references whenever possible:
https://www.cs.rit.edu/~mjh/docs/c++-faq/references.html#faq-8.6

This is also what the C++ Core Guidelines recommend, i.e., to
declare "in arguments" by const reference and "in-out" arguments
by non-const reference:

https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rf-inout

> > The warnings above use "attribute %<fallthrough%>" yet the notes here
> > use "__attribute__ ((fallthrough));"  It seems that using consistent
> > spelling in all messages would be better (and avoid emphasizing one
> > spelling of the attribute over others, especially the C++ 17
> > [[fallthrough]]).
>
> Here I think I'd rather not change things.  I think what I currently have is
> fine, because the fixit hints are meants to be verbatim code, the warnings
> talk about the attribute more generally.

Understood.  But suggesting __attribute__((fallthrough)) in C++ 17
code guides users toward writing non-portable code.  I think GCC
should recommend a standard solution first, and only suggest
extensions where there is no standard alternative.  One way to do
it would be to issue different hints based on the target standard.
To avoid complicating the code with conditionals the issue can be
circumvented to by using "attribute %<fallthrough%>" here as well.

FWIW, this seems like another broader topic to have consensus
on and document: GCC messages should be consistent not only in
appearance (quoting, etc.) but also (and I'd say even more
important) in the spelling of alternate language constructs
they suggest.

Martin