This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: cp: implementation of p1301 for C++


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]