Just a few minor nits based on a quick reading of the patch:
@@ -24114,6 +24164,16 @@ cp_parser_std_attribute (cp_parser *parser)
" use %<gnu::deprecated%>");
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,
+ "%<fallthrough%> 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.
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.
+ " use %<gnu::fallthrough%>");
+ 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).
+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...
(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.)
+/* Find LABEL in vector of label entries VEC. */
+
+static struct label_entry *
+find_label_entry (const auto_vec<struct label_entry> *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> &cases, 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.)
+/* Collect interesting labels to LABELS and return the statement preceding
+ another case label, or a user-defined label. */
Suggest either "Collect interesting labels in LABELS" or "Append
interesting labels to LABELS..."
+
+static gimple *
+collect_fallthrough_labels (gimple_stmt_iterator *gsi_p,
+ auto_vec <struct label_entry> *const labels)
The const above seems superfluous (it says the labels pointer
doesn't change, which could be helpful to the reader of the code,
but then it suggests that gsi_p does change, which isn't the case).
+ /* Suggestion one: add "__attribute__ ((fallthrough));". */
+ rich_location rloc_attr (line_table, gimple_location (next));
+ rloc_attr.add_fixit_insert (gimple_location (next),
+ "__attribute__ ((fallthrough));");
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]]).
+static void
+expand_FALLTHROUGH (internal_fn, gcall *call)
+{
+ error_at (gimple_location (call),
+ "invalid use of %<__attribute__((fallthrough));%>");
Similar to my comment above, I would suggest using the same spelling
for the attribute in all messages.
+static bool
+fallthrough_comment_p (cpp_reader *pfile, const unsigned char *comment_start)
+{
+ const unsigned char *from = comment_start + 1;
+ /* Whole comment contents:
+ -fallthrough
+ @fallthrough@
+ */
+ if (*from == '-' || *from == '@')
+ {
+ size_t l = sizeof "fallthrough" - 1;
FWIW, I would suggest avoiding the letter 'l' for an identifier
It's easy to misread it as the digit '1' (I had initially misread
it in the memcmp call below that way and had to back track up here).
Martin
