Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PATCH] PERL_UNUSED_RESULT: hopefully better way to ignore results (ditch V_Gconvert) #13781

Closed
p5pRT opened this issue Apr 29, 2014 · 8 comments
Labels

Comments

@p5pRT
Copy link

p5pRT commented Apr 29, 2014

Migrated from rt.perl.org#121763 (status was 'resolved')

Searchable as RT121763$

@p5pRT
Copy link
Author

p5pRT commented Apr 29, 2014

From @jhi

I like having less noise in my OS X build... the V_Gconvert isn't
working well for that since my Gconvert() isn't returning a char*.

Blurb from comment​:

+/* Use PERL_UNUSED_RESULT() to suppress the warnings about unused results
+ * of function calls, e.g. PERL_UNUSED_RESULT(foo(a, b)). Use it
sparingly,
+ * though, since usually the warning is there for a good reason,
+ * e.g. for realloc()​: the new pointer is not necessarily the old pointer.
+ *
+ * But sometimes you just want to ignore the return value, e.g. on
+ * codepaths soon ending up in abort, or in "best effort" attempts.
+ * Sometimes you can capture the return value and use PERL_UNUSED_VAR
+ * on that.
+ *
+ * The combination of gcc -Wunused-result (part of -Wall) and the gcc
+ * warn_unused_result attribute cannot be silenced with (void).
+ *
+ * The __typeof__() is unused instead of typeof() since typeof() is
+ * not available under stricter ANSI modes, and because of compilers
+ * masquerading as gcc (clang and icc), we want exactly the gcc
+ * extension __typeof__ and nothing else.
+ */

Patch attached.

@p5pRT
Copy link
Author

p5pRT commented Apr 29, 2014

From @jhi

0001-Add-PERL_UNUSED_RESULT-and-use-that-instead-of-the-V.patch
From 062f092deef28f97906bfb6781a03786daecf5ab Mon Sep 17 00:00:00 2001
From: Jarkko Hietaniemi <jhi@iki.fi>
Date: Sun, 27 Apr 2014 20:52:54 -0400
Subject: [PATCH] Add PERL_UNUSED_RESULT() and use that instead of the
 V_Gconvert().

---
 perl.h | 26 ++++++++++++++++++++++++++
 sv.c   | 19 +++++--------------
 2 files changed, 31 insertions(+), 14 deletions(-)

diff --git a/perl.h b/perl.h
index 6da39f3..b740d9a 100644
--- a/perl.h
+++ b/perl.h
@@ -327,6 +327,32 @@
 #  define PERL_UNUSED_CONTEXT
 #endif
 
+/* Use PERL_UNUSED_RESULT() to suppress the warnings about unused results
+ * of function calls, e.g. PERL_UNUSED_RESULT(foo(a, b)).  Use it sparingly,
+ * though, since usually the warning is there for a good reason,
+ * e.g. for realloc(): the new pointer is not necessarily the old pointer.
+ *
+ * But sometimes you just want to ignore the return value, e.g. on
+ * codepaths soon ending up in abort, or in "best effort" attempts.
+ * Sometimes you can capture the return value and use PERL_UNUSED_VAR
+ * on that.
+ *
+ * The combination of gcc -Wunused-result (part of -Wall) and the gcc
+ * warn_unused_result attribute cannot be silenced with (void).
+ *
+ * The __typeof__() is unused instead of typeof() since typeof() is
+ * not available under stricter ANSI modes, and because of compilers
+ * masquerading as gcc (clang and icc), we want exactly the gcc
+ * extension __typeof__ and nothing else.
+ */
+#ifndef PERL_UNUSED_RESULT
+#  ifdef __GNUC__
+#    define PERL_UNUSED_RESULT(v) ({ __typeof__(v) z = (v); (void)sizeof(z); })
+#  else
+#    define PERL_UNUSED_RESULT(v) ((void)(v))
+#  endif
+#endif
+
 /* on gcc (and clang), specify that a warning should be temporarily
  * ignored; e.g.
  *
diff --git a/sv.c b/sv.c
index 087606b..a35b011 100644
--- a/sv.c
+++ b/sv.c
@@ -50,13 +50,6 @@
  * has a mandatory return value, even though that value is just the same
  * as the buf arg */
 
-#define V_Gconvert(x,n,t,b) \
-{ \
-    char *rc = (char *)Gconvert(x,n,t,b); \
-    PERL_UNUSED_VAR(rc); \
-}
-
-
 #ifdef PERL_UTF8_CACHE_ASSERT
 /* if adding more checks watch out for the following tests:
  *   t/op/index.t t/op/length.t t/op/pat.t t/op/substr.t
@@ -2954,12 +2947,12 @@ Perl_sv_2pv_flags(pTHX_ SV *const sv, STRLEN *const lp, const I32 flags)
 	    /* some Xenix systems wipe out errno here */
 
 #ifndef USE_LOCALE_NUMERIC
-            V_Gconvert(SvNVX(sv), NV_DIG, 0, s);
+            PERL_UNUSED_RESULT(Gconvert(SvNVX(sv), NV_DIG, 0, s));
             SvPOK_on(sv);
 #else
             {
                 DECLARE_STORE_LC_NUMERIC_SET_TO_NEEDED();
-                V_Gconvert(SvNVX(sv), NV_DIG, 0, s);
+                PERL_UNUSED_RESULT(Gconvert(SvNVX(sv), NV_DIG, 0, s));
 
                 /* If the radix character is UTF-8, and actually is in the
                  * output, turn on the UTF-8 flag for the scalar */
@@ -10547,7 +10540,7 @@ Perl_sv_vcatpvfn_flags(pTHX_ SV *const sv, const char *const pat, const STRLEN p
 		if (digits && digits < sizeof(ebuf) - NV_DIG - 10) {
 		     /* 0, point, slack */
                     STORE_LC_NUMERIC_SET_TO_NEEDED();
-		    V_Gconvert(nv, (int)digits, 0, ebuf);
+		    PERL_UNUSED_RESULT(Gconvert(nv, (int)digits, 0, ebuf));
 		    sv_catpv_nomg(sv, ebuf);
 		    if (*ebuf)	/* May return an empty string for digits==0 */
 			return;
@@ -11405,7 +11398,7 @@ Perl_sv_vcatpvfn_flags(pTHX_ SV *const sv, const char *const pat, const STRLEN p
 		   aka precis is 0  */
 		if ( c == 'g' && precis) {
                     STORE_LC_NUMERIC_SET_TO_NEEDED();
-		    V_Gconvert((NV)nv, (int)precis, 0, PL_efloatbuf);
+		    PERL_UNUSED_RESULT(Gconvert((NV)nv, (int)precis, 0, PL_efloatbuf));
 		    /* May return an empty string for digits==0 */
 		    if (*PL_efloatbuf) {
 			elen = strlen(PL_efloatbuf);
@@ -11836,7 +11829,6 @@ Perl_dirp_dup(pTHX_ DIR *const dp, CLONE_PARAMS *const param)
     DIR *ret;
 
 #if defined(HAS_FCHDIR) && defined(HAS_TELLDIR) && defined(HAS_SEEKDIR)
-    int rc = 0;
     DIR *pwd;
     const Direntry_t *dirent;
     char smallbuf[256];
@@ -11873,9 +11865,8 @@ Perl_dirp_dup(pTHX_ DIR *const dp, CLONE_PARAMS *const param)
     /* Now we should have two dir handles pointing to the same dir. */
 
     /* Be nice to the calling code and chdir back to where we were. */
-    rc = fchdir(my_dirfd(pwd));
     /* XXX If this fails, then what? */
-    PERL_UNUSED_VAR(rc);
+    PERL_UNUSED_VAR(fchdir(my_dirfd(pwd)));
 
     /* We have no need of the pwd handle any more. */
     PerlDir_close(pwd);
-- 
1.9.2

@p5pRT
Copy link
Author

p5pRT commented May 28, 2014

From @tsee

On Tue Apr 29 05​:33​:40 2014, jhi wrote​:

I like having less noise in my OS X build... the V_Gconvert isn't
working well for that since my Gconvert() isn't returning a char*.

Reading the patch, there's two hunks at the end that look like this​:

@​@​ -11836,7 +11829,6 @​@​ Perl_dirp_dup(pTHX_ DIR *const dp, CLONE_PARAMS *const param)
  DIR *ret;

#if defined(HAS_FCHDIR) && defined(HAS_TELLDIR) && defined(HAS_SEEKDIR)
- int rc = 0;
  DIR *pwd;
  const Direntry_t *dirent;
  char smallbuf[256];
@​@​ -11873,9 +11865,8 @​@​ Perl_dirp_dup(pTHX_ DIR *const dp, CLONE_PARAMS *const param)
  /* Now we should have two dir handles pointing to the same dir. */

  /* Be nice to the calling code and chdir back to where we were. */
- rc = fchdir(my_dirfd(pwd));
  /* XXX If this fails, then what? */
- PERL_UNUSED_VAR(rc);
+ PERL_UNUSED_VAR(fchdir(my_dirfd(pwd)));

  /* We have no need of the pwd handle any more. */
  PerlDir_close(pwd);

Did you mean to use your PERL_UNUSED_RESULT instead of PERL_UNUSED_VAR on this?

I've applied the patch locally as is since I believe it's technically correct, but if the above was a brain fart, feel free to push a follow-up.

@p5pRT
Copy link
Author

p5pRT commented May 28, 2014

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

p5pRT commented May 28, 2014

From @jhi

On Wednesday-201405-28, 4​:32, Steffen Mueller via RT wrote​:

Did you mean to use your PERL_UNUSED_RESULT instead of PERL_UNUSED_VAR on this?

I've applied the patch locally as is since I believe it's technically correct, but if the above was a brain fart, feel free to push a follow-up.

Brainfart, yes. I meant _RESULT.

@p5pRT
Copy link
Author

p5pRT commented May 28, 2014

From @tsee

On 05/28/2014 12​:38 PM, Jarkko Hietaniemi wrote​:

On Wednesday-201405-28, 4​:32, Steffen Mueller via RT wrote​:

Did you mean to use your PERL_UNUSED_RESULT instead of PERL_UNUSED_VAR
on this?

I've applied the patch locally as is since I believe it's technically
correct, but if the above was a brain fart, feel free to push a
follow-up.

Brainfart, yes. I meant _RESULT.

Will fix.

--Steffen

@p5pRT
Copy link
Author

p5pRT commented May 28, 2014

From @tsee

Thanks applied (with the discussed amendment) as ffea512.

@p5pRT p5pRT closed this as completed May 28, 2014
@p5pRT
Copy link
Author

p5pRT commented May 28, 2014

@tsee - Status changed from 'open' to 'resolved'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant