Re: Implement -Wimplicit-fallthrough (version 5)

[Martin Sebor]

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, );

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 %" 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 %" 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

Re: Implement -Wimplicit-fallthrough (version 5)

[Marek Polacek]

On Mon, Aug 22, 2016 at 10:31:16AM -0600, Martin Sebor wrote:
> Just a few minor nits based on a quick reading of the patch:

Thanks for looking into this.
 
> > @@ -24114,6 +24164,16 @@ cp_parser_std_attribute (cp_parser *parser)
> >  " use %");
> >   TREE_PURPOSE (TREE_PURPOSE (attribute)) = get_identifier ("gnu");
> > }
> > +  /* C++17 fallthrough attribute is equivalent to GNU's.  */
> > +  else if (cxx_dialect >= cxx11
> > +  && is_attribute_p ("fallthrough", attr_id))
> > +   {
> > + if (cxx_dialect < cxx1z)
> > +   pedwarn (token->location, OPT_Wpedantic,
> > +"% is a C++17 feature;"
> 
> The text of the message above is missing the word "attribute" that's
> normally used in all other diagnostics about attributes.
 
Here I copied over the C++14 deprecated attribute handling above and
that doesn't have the word "attribute".  I guess we can fix both later.

> But I wonder about issuing a pedantic warning for this C++ 17 attribute
> when it isn't issued for others such as nodiscard.  I would expect them
> to be diagnosed consistently (i.e., __attribute__((fallthrough)) to be
> accepted in all modes, and [[fallthrough]] diagnosed in C++ 14 mode,
> and all [[...]] attributes diagnosed in C++ 98 as they are now).  It
> looks to me like your patch is close to what I expect but different
> from other C++ 17 attributes.
 
So currently __attribute__((fallthrough)) is accepted in all modes,
[[fallthrough]] diagnosed in C++14 mode and all [[...]] attributes diagnosed in
C++98 mode -- no disagreement on that.  But I think the pedwarn for the
fallthrough attribute it appropriate, and we should also diagnose nodiscard.
And maybe_unused too.  Perhaps we'll need to discuss this with Jason, though
it probably isn't the most important thing.
 
> > +" use %");
> > + TREE_PURPOSE (TREE_PURPOSE (attribute)) = get_identifier ("gnu");
> > +   }
> > /* Transactional Memory TS optimize_for_synchronized attribute is
> >  equivalent to GNU transaction_callable.  */
> > else if (is_attribute_p ("optimize_for_synchronized", attr_id))
> > @@ -24178,6 +24238,10 @@ cp_parser_check_std_attribute (tree attributes, 
> > tree attribute)
> >&& lookup_attribute ("deprecated", attributes))
> > error ("attribute deprecated can appear at most once "
> >"in an attribute-list")
> > +  else if (is_attribute_p ("fallthrough", name)
> > +  && lookup_attribute ("fallthrough", attributes))
> > +   error ("attribute fallthrough can appear at most once "
> > +  "in an attribute-list");
> 
> Here "fallthrough" isn't quoted when it is everywhere else as far
> as I noticed (looks like the same applies to "deprecated" above).
 
I fixed all of them byt adding %<...%>.

> > +C++17 provides a standard way to suppress the 
> > @option{-Wimplicit-fallthrough}
> > +warning using @code{[[fallthrough]];} instead of the GNU attribute.  In 
> > C++11
> > +or C++14 users can use @code{[[gnu::fallthrough]];}, which is a GNU 
> > extension.
> > +Instead of the these attributes, it is also possible to add a "falls 
> > through"
> > +comment to silence the warning.  GCC accepts wide range of such comments, 
> > so
> > +e.g. all of "Falls through.", "fallthru", "FALLS-THROUGH" work.
> 
> The last sentence is missing an article:
> 
>   GCC accepts _a_ wide range of such comments...
 
Fixed.

> (Personally, I also find using "e.g." in the middle of a sentence
> a bit too informal and would like it better spelled out, but that
> could just be me.)
 
Changed to "for example", if that's any better.

> > +/* Find LABEL in vector of label entries VEC.  */
> > +
> > +static struct label_entry *
> > +find_label_entry (const auto_vec *vec, tree label)
> > +{
> > +  unsigned int i;
> > +  struct label_entry *l;
> > +
> > +  FOR_EACH_VEC_ELT (*vec, i, l)
> > +if (l->label == label)
> > +  return l;
> > +  return NULL;
> > +}
> > +
> > +/* Return true if LABEL, a LABEL_DECL, represents a case label
> > +   in a vector of labels CASES.  */
> > +
> > +static bool
> > +case_label_p (const vec , tree label)
> 
> 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.

> > +/* Collect interesting labels to LABELS and return the statement preceding
> > +   another case label, or a user-defined label.  */
> 
> Suggest either "Collect