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

sv.c: Eliminate LGTM warning Comparison result is always the same #16798

Closed
p5pRT opened this issue Dec 30, 2018 · 11 comments
Closed

sv.c: Eliminate LGTM warning Comparison result is always the same #16798

p5pRT opened this issue Dec 30, 2018 · 11 comments

Comments

@p5pRT
Copy link

p5pRT commented Dec 30, 2018

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

Searchable as RT133744$

@p5pRT
Copy link
Author

p5pRT commented Dec 30, 2018

From @jkeenan

In RT 133703, I attempted to address the result of a finding from the
lgtm.com analysis of the source code in sv.c.

Links​:
https://lgtm.com/rules/2154840804/

lgtm rule for comparisons always giving same result​:
https://lgtm.com/rules/2154840804/

That ticket stalled when it became apparent that we didn't have a
sufficiently clear understanding of the so-called "paragraph mode"

#####
$/ = "";
#####

... and that that functionality was undertested. We provided better
documentation and more tests in RT 133722. So now we should be in a
better position to apply a patch along the lines suggested by Dave
Mitchell in 133703.

Please review the patch attached, which is smoking in this branch​:

smoke-me/jkeenan/davem/sv-c-comparison-same

Thank you very much.
Jim Keenan

@p5pRT
Copy link
Author

p5pRT commented Dec 30, 2018

From @jkeenan

This is perl 5, version 29, subversion 7 (v5.29.7 (v5.29.6-69-gc011174cae)) built for x86_64-linux
(with 93 registered patches, see perl -V for more detail)

Copyright 1987-2018, Larry Wall

Perl may be copied only under the terms of either the Artistic License or the
GNU General Public License, which may be found in the Perl 5 source kit.

Complete documentation for Perl, including FAQ lists, should be found on
this system using "man perl" or "perldoc perl". If you have access to the
Internet, point your browser at http​://www.perl.org/, the Perl Home Page.

@p5pRT
Copy link
Author

p5pRT commented Dec 30, 2018

From @jkeenan

0001-If-comparison-is-always-true-while-1-should-suffice.patch
From c011174cae6c7c1b06b39a053787028a1c9fa7cd Mon Sep 17 00:00:00 2001
From: James E Keenan <jkeenan@cpan.org>
Date: Fri, 30 Nov 2018 10:33:07 -0500
Subject: [PATCH] If comparison is always true, while (1) should suffice.

Per suggestion by Dave Mitchell in RT 133703.
---
 sv.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/sv.c b/sv.c
index bc06c6c659..d9a0f41689 100644
--- a/sv.c
+++ b/sv.c
@@ -8535,18 +8535,19 @@ Perl_sv_gets(pTHX_ SV *const sv, PerlIO *const fp, I32 append)
      * null assign is a placeholder. */
     rslast = rslen ? rsptr[rslen - 1] : '\0';
 
-    if (rspara) {		/* have to do this both before and after */
-	do {			/* to make sure file boundaries work right */
-	    if (PerlIO_eof(fp))
-		return 0;
-	    i = PerlIO_getc(fp);
-	    if (i != '\n') {
-		if (i == -1)
-		    return 0;
-		PerlIO_ungetc(fp,i);
-		break;
-	    }
-	} while (i != EOF);
+    if (rspara) {        /* have to do this both before and after */
+                         /* to make sure file boundaries work right */
+        while (1) {
+            if (PerlIO_eof(fp))
+                return 0;
+            i = PerlIO_getc(fp);
+            if (i != '\n') {
+                if (i == -1)
+                    return 0;
+                PerlIO_ungetc(fp,i);
+                break;
+            }
+        }
     }
 
     /* See if we know enough about I/O mechanism to cheat it ! */
-- 
2.17.1

@p5pRT
Copy link
Author

p5pRT commented Jan 2, 2019

From @iabyn

On Sun, Dec 30, 2018 at 07​:49​:34AM -0800, James E Keenan (via RT) wrote​:

... and that that functionality was undertested. We provided better
documentation and more tests in RT 133722. So now we should be in a
better position to apply a patch along the lines suggested by Dave
Mitchell in 133703.

Please review the patch attached, which is smoking in this branch​:

smoke-me/jkeenan/davem/sv-c-comparison-same

Can we ditch the gratuitous whitespace changes on otherwise unaffected
lines, please!

--
Monto Blanco... scorchio!

@p5pRT
Copy link
Author

p5pRT commented Jan 2, 2019

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

@p5pRT
Copy link
Author

p5pRT commented Jan 5, 2019

From @xsawyerx

 
 
 
 
 
 

  On 1/2/19 10​:57 AM, Dave Mitchell
  wrote​:

 
 
  On Sun, Dec 30, 2018 at 07​:49​:34AM -0800, James E Keenan (via RT) wrote​:

 
  ... and that that functionality was undertested. We provided better
documentation and more tests in RT 133722. So now we should be in a
better position to apply a patch along the lines suggested by Dave
Mitchell in 133703.

Please review the patch attached, which is smoking in this branch​:

smoke-me/jkeenan/davem/sv-c-comparison-same

 
 
Can we ditch the gratuitous whitespace changes on otherwise unaffected
lines, please!
 
 

 
  Thank you, Jim, for providing a necessary patch with a fix.

 

  (In the future, commits for whitespace are best done separately to
  reduce the workload when reviewing them. It makes them easier to
  apply, rebase, and merge.)

 

  Dave, the whitespace is not your enemy. (Well, maybe it is... but
  Jim isn't!) :)

 
 

@p5pRT
Copy link
Author

p5pRT commented Jan 29, 2019

From @tonycoz

On Sun, 30 Dec 2018 07​:49​:34 -0800, jkeenan@​pobox.com wrote​:

Please review the patch attached, which is smoking in this branch​:

It looks reasonable to me.

Tony

@p5pRT
Copy link
Author

p5pRT commented Jan 30, 2019

From @jkeenan

On Tue, 29 Jan 2019 23​:30​:32 GMT, tonyc wrote​:

On Sun, 30 Dec 2018 07​:49​:34 -0800, jkeenan@​pobox.com wrote​:

Please review the patch attached, which is smoking in this branch​:

It looks reasonable to me.

Tony

Thanks. Merged to blead in commit e8db349.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

p5pRT commented Jan 30, 2019

@jkeenan - Status changed from 'open' to 'pending release'

@p5pRT
Copy link
Author

p5pRT commented May 22, 2019

From @khwilliamson

Thank you for filing this report. You have helped make Perl better.

With the release today of Perl 5.30.0, this and 160 other issues have been
resolved.

Perl 5.30.0 may be downloaded via​:
https://metacpan.org/release/XSAWYERX/perl-5.30.0

If you find that the problem persists, feel free to reopen this ticket.

@p5pRT
Copy link
Author

p5pRT commented May 22, 2019

@khwilliamson - Status changed from 'pending release' to 'resolved'

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

No branches or pull requests

1 participant