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 #16776
Comments
From @jkeenanThis ticket will hold a patch intended to suppress a warning reporting The relevant part of sv.c is: ##### Warning: "Comparison result is always the same" I'll attach the patch once I get an RT number. Thank you very much. |
From @jkeenanSummary of my perl5 (revision 5 version 29 subversion 6) configuration: Characteristics of this binary (from libperl): |
From @jkeenanOn Thu, 29 Nov 2018 20:47:22 GMT, jkeenan@pobox.com wrote:
Branch: smoke-me/jkeenan/133703-sv-c Patch: 133703-0001-If-comparison-is-always-true-no-need-for-do-while-lo.patch Note: While the branch has been passing its test, we have to be alert to the possibility that the test suite might never have exercised the code in question. Thank you very much. |
From @jkeenan133703-0001-If-comparison-is-always-true-no-need-for-do-while-lo.patchFrom 3bef985dd028c3af8cfd667cf164452c83159fad Mon Sep 17 00:00:00 2001
From: James E Keenan <jkeenan@cpan.org>
Date: Tue, 27 Nov 2018 22:08:40 -0500
Subject: [PATCH] If comparison is always true, no need for do-while loop.
LGTM analysis: https://lgtm.com/projects/g/Perl/perl5/alerts/?mode=tree&ruleFocus=2154840804
LGTM rule: https://lgtm.com/rules/2154840804/
---
sv.c | 22 ++++++++++------------
1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/sv.c b/sv.c
index 826228bc92..73b0659d7d 100644
--- a/sv.c
+++ b/sv.c
@@ -8483,18 +8483,16 @@ 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 */
+ if (PerlIO_eof(fp))
+ return 0;
+ i = PerlIO_getc(fp);
+ if (i != '\n') {
+ if (i == -1)
+ return 0;
+ PerlIO_ungetc(fp,i);
+ }
}
/* See if we know enough about I/O mechanism to cheat it ! */
--
2.17.1
|
From [Unknown Contact. See original ticket]On Thu, 29 Nov 2018 20:47:22 GMT, jkeenan@pobox.com wrote:
Branch: smoke-me/jkeenan/133703-sv-c Patch: 133703-0001-If-comparison-is-always-true-no-need-for-do-while-lo.patch Note: While the branch has been passing its test, we have to be alert to the possibility that the test suite might never have exercised the code in question. Thank you very much. |
From @iabynOn Thu, Nov 29, 2018 at 12:53:11PM -0800, James E Keenan via RT wrote:
That doesn't look right - you've completely removed the while loop, which Its odd that it doesn't trigger any test failures - its possible that some The condition which the code analyser was complaining about is that do { or more simply, while (1) { -- |
@jkeenan - Status changed from 'new' to 'open' |
From @jkeenanOn Thu, 29 Nov 2018 23:19:36 GMT, davem wrote:
I agree that the absence of test breakage is puzzling. To investigate further, I wrote an additional program, 133703-paragraph-mode.t, attached. This program PASSes in perl-5.28.0 and in my branch. I then created another branch, jkeenan/davem/133703-sv-c, in which I implemented your suggestion to replace the do..while loop with a simple while(1) loop. This branch PASSes both 133703-paragraph-mode.t and 'make test_harness'. The fact that everything is PASSing is puzzling. Thank you very much. |
From @jkeenan133703-0001-If-comparison-is-always-true-while-1-should-suffice.patchFrom 06e2209b8023d04c7fae3cc01e4aba4bc279dae0 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 826228bc92..3dc5a4fe8d 100644
--- a/sv.c
+++ b/sv.c
@@ -8483,18 +8483,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
|
From @jkeenanuse strict; my ($fh, $filename) = tempfile(); open my $OUT, '>', $filename or die; my @expected = ( my @got; is_deeply(\@got, \@expected, q|$/ = "" -- Got expected paragraphs|) done_testing(); |
From @jkeenanOn Fri, 30 Nov 2018 15:56:08 GMT, jkeenan wrote:
In sv.c in blead we have: ##### The last subsection above is the one where LGTM saw an unnecessary comparison. But then further down, towards the end of the definition of Perl_sv_gets, we have: ##### Is it possible that the test suite (e.g., t/base/rs.t) and my additional program are only exercising the latter block? Thank you very much. -- |
From @LeontOn Fri, Nov 30, 2018 at 5:14 PM James E Keenan via RT
Yes. The first section is before the actual readline code, and consumes any We clearly should add a test for that. Leon |
From @jkeenanOn Fri, 30 Nov 2018 16:37:31 GMT, LeonT wrote:
Please see the latest attachment, 133703-more-paragraph-mode-tests.t. I believe this code exercises the 'if' block at line 8486 of sv.3 in blead.* If in the test file I have set up the expected results correctly, then we have a bug. The output I get from running this test either with perl-5.28.0 (via 'prove') or blead is: ##### Test Summary Report 133703-more-paragraph-mode-tests.t (Wstat: 2560 Tests: 12 Failed: 10) Thank you very much. -- |
From @jkeenan#!/usr/bin/env perl my $chunks = [ 1, 12345, 123456 ]; $teststring = "\n\n\n$chunks->[0]\n$chunks->[1]\n\n$chunks->[2]\n"; $teststring = "\n\n\n$chunks->[0]\n\n$chunks->[1]\n\n$chunks->[2]\n"; $teststring = "\n\n\n$chunks->[0]\n\n\n$chunks->[1]\n\n$chunks->[2]\n"; $teststring = "\n\n\n$chunks->[0]\n$chunks->[1]\n\n$chunks->[2]\n\n"; done_testing(); sub test_paragraph_mode { open my $OUT, '>', $filename or die; local $/ = ""; # paragraph mode |
From @jkeenanOn Fri, 30 Nov 2018 23:36:06 GMT, jkeenan wrote:
The last test file I attached had some incorrect expectations. Please see the latest attachment, 133703-better-more-paragraph-mode-tests.t, which accounts for the fact that our definition of "paragraph" assumes that the 'graphs are separated by 2 or more newlines but may internally contain single newlines. My impression is that "local $/ = '';" fails to strip superfluous newlines off the *first* paragraph it finds -- but this needs more eyeballs. Thank you very much. -- |
From @jkeenan#!/usr/bin/env perl my $chunks = [ 1, 12345, 123456 ]; $teststring = "\n\n\n$chunks->[0]\n\n$chunks->[1]\n\n$chunks->[2]\n"; $teststring = "\n\n\n$chunks->[0]\n\n$chunks->[1]\n\n\n$chunks->[2]\n"; $teststring = "\n\n\n$chunks->[0]\n\n\n$chunks->[1]\n\n$chunks->[2]\n"; $teststring = "\n\n\n$chunks->[0]\n\n$chunks->[1]\n\n$chunks->[2]"; note("XXX: Paragraphs with internal newlines"); $teststring = "\n\n\n$chunks->[0]\n$chunks->[1]\n\n$chunks->[2]\n"; $teststring = "\n\n\n$chunks->[0]\n$chunks->[1]\n\n\n$chunks->[2]\n"; $teststring = "\n\n\n$chunks->[0]\n$chunks->[1]\n\n$chunks->[2]"; done_testing(); ##### SUBROUTINES ##### sub test_paragraph_mode { open my $OUT, '>', $filename or die; local $/ = ""; # paragraph mode sub test_paragraph_mode_internal_newline { open my $OUT, '>', $filename or die; local $/ = ""; # paragraph mode sub simulate_paragraph_mode { |
From @jkeenanOn Sat, 01 Dec 2018 04:49:35 GMT, jkeenan wrote:
Closing this ticket and replacing it with https://rt-archive.perl.org/perl5/Ticket/Display.html?id=133722. Thank you very much. |
@jkeenan - Status changed from 'open' to 'rejected' |
Migrated from rt.perl.org#133703 (status was 'rejected')
Searchable as RT133703$
The text was updated successfully, but these errors were encountered: