On 7/20/19 11:29 AM, JeanHeyd Meneide wrote:
Dear GCC Community,
This patch implements the recently accepted p1301: [[nodiscard("should
have a reason")]]. Aaron Ballman implemented it for Clang in
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20190715/280158.html
-- this is in preparation for a paper that will soon go to the C Committee
to keep feature-parity with C++ (the C2x draft already has attributes with
syntax exactly like C++).
Comments welcome: this is my first patch, and probably needs a lot of
help. This is also part of my Google Summer of Code training, to get used
to submitting and sending patches on the gcc-patches list.
Just a few minor notes/suggestions.
...
--- a/gcc/cp/cvt.c
+++ b/gcc/cp/cvt.c
@@ -35,6 +35,7 @@ along with GCC; see the file COPYING3. If not see
#include "convert.h"
#include "stringpool.h"
#include "attribs.h"
+#include "escaped_string.h"
static tree convert_to_pointer_force (tree, tree, tsubst_flags_t);
static tree build_type_conversion (tree, tree);
@@ -1029,19 +1030,29 @@ maybe_warn_nodiscard (tree expr, impl_conv_void
implicit)
if (implicit != ICV_CAST && fn
&& lookup_attribute ("nodiscard", DECL_ATTRIBUTES (fn)))
{
- auto_diagnostic_group d;
+ tree attr = DECL_ATTRIBUTES (fn);
+ escaped_string msg;
+ if (attr)
+ msg.escape (TREE_STRING_POINTER (TREE_VALUE (TREE_VALUE (attr))));
+ bool has_msg = static_cast<bool>(msg);
+ auto_diagnostic_group d;
if (warning_at (loc, OPT_Wunused_result,
- "ignoring return value of %qD, "
- "declared with attribute nodiscard", fn))
- inform (DECL_SOURCE_LOCATION (fn), "declared here");
+ "ignoring return value of %qD, "
+ "declared with attribute nodiscard%s%s", fn, (has_msg
? ": " : ""), (has_msg ? (const char*)msg : "")))
+ inform (DECL_SOURCE_LOCATION (fn), "declared here");
While making changes here, would you mind adding quotes around
nodiscard (i.e., %<nodiscard%>). Should the text included in
the attribute also be similarly quoted so that it's highlighted
in the diagnostic?
}
else if (implicit != ICV_CAST
&& lookup_attribute ("nodiscard", TYPE_ATTRIBUTES (rettype)))
{
+ tree attr = TYPE_ATTRIBUTES (rettype);
+ escaped_string msg;
+ if (attr)
+ msg.escape (TREE_STRING_POINTER (TREE_VALUE (TREE_VALUE (attr))));
+ bool has_msg = static_cast<bool>(msg);
auto_diagnostic_group d;
if (warning_at (loc, OPT_Wunused_result,
- "ignoring returned value of type %qT, "
- "declared with attribute nodiscard", rettype))
+ "ignoring returned value of type %qT, "
+ "declared with attribute nodiscard%s%s", rettype,
Also here.
(has_msg ? ": " : ""), (has_msg ? (const char*)msg : "")))
{
if (fn)
inform (DECL_SOURCE_LOCATION (fn),
...
diff --git a/gcc/cp/tree.c b/gcc/cp/tree.c
index 37e24a1669c..8c2d056f3eb 100644
--- a/gcc/cp/tree.c
+++ b/gcc/cp/tree.c
@@ -4356,9 +4356,27 @@ zero_init_p (const_tree t)
warn_unused_result attribute. */
static tree
-handle_nodiscard_attribute (tree *node, tree name, tree /*args*/,
+handle_nodiscard_attribute (tree *node, tree name, tree args,
int /*flags*/, bool *no_add_attrs)
{
+ if (!args)
+ *no_add_attrs = true;
+ else if (cxx_dialect >= cxx2a)
+ {
+ if (TREE_CODE (TREE_VALUE (args)) != STRING_CST)
+ {
+ error ("nodiscard attribute argument must be a string");
And also here; to reduce the number of format strings that have to
be translated it's best to use:
error ("%qE attribute argument must be a string constant", name);
+ *no_add_attrs = true;
+ }
+ }
+ else
+ {
+ if (!*no_add_attrs)
+ {
+ error("nodiscard attribute does not take any arguments: use flag
%<-std=c2a%> or better to compile your code");
Also here. Rather than a colon I think the dominant style is to
use a semicolon but I would suggest:
error ("%E attribute with an argument only available with "
"%<-std=c++2a%> or %<-std=gnu++2a%>");
in keeping with other messages like it (see gcc/po/gcc.pot for
examples). (I assume the option to enable the C++ 2a dialect
is -std=c++2a, not -std=c2a.)
+++ b/gcc/escaped_string.h
@@ -0,0 +1,41 @@
...
+/* A class to handle converting a string that might contain
+ control characters, (eg newline, form-feed, etc), into one
+ in which contains escape sequences instead. */
+
+class escaped_string
+{
+ public:
+ escaped_string () { m_owned = false; m_str = NULL; };
+ ~escaped_string () { if (m_owned) free (m_str); }
+ operator const char *() const { return (const char *) m_str; }
+ void escape (const char *);
+ private:
+ char *m_str;
+ bool m_owned;
+};
Since the class isn't safe to copy/assign and it's being moved
to a header, could you please make its copy ctor and assignment
operator private to prevent its objects from accidentally
getting copied and corrupting memory?
...
+++ b/gcc/testsuite/g++.dg/cpp2a/nodiscard-bad-clause.C
@@ -0,0 +1,12 @@
+/* nodiscard attribute tests */
+/* { dg-do compile { target c++2a } } */
+/* { dg-options "-O -ftrack-macro-expansion=0" } */
+
+[[nodiscard(123)]] int check1 (void); /* { dg-error "nodiscard attribute
argument.*must be a string" } */
Using .* might be safe in a test with a single line of output but
not in other tests where it might consume newlines. It's best to
Martin
