Mailing List Archive: Perl: porters

[perl #3330] Magic increment avoids warning unexpectedly

Index | Next | Previous | View Threaded

perlbug-followup at perl

Aug 4, 2013, 11:48 PM

Post #1 of 5 (21 views)
Permalink

[perl #3330] Magic increment avoids warning unexpectedly

On Tue Jul 09 22:30:15 2013, tonyc wrote:
> On Sat Jul 06 12:46:41 2013, perl.p5p [at] rjbs wrote:
> > Yes, I think so.
>
> I'll produce a better patch for it sometime.
>
> One problem with my patch above is it complains "Argument 'foo!' isn't
> numeric..." but ++ accepts non-numeric arguments, so the message needs
> to change.

Here's an improved change with tests and a different message.

Tony

---
via perlbug: queue: perl5 status: open
https://rt.perl.org:443/rt3/Ticket/Display.html?id=3330

Attachments:

0001-perl-3330-warn-on-increment-of-an-non-number-non-mag.patch (7.18 KB)

perlbug-followup at perl

Aug 5, 2013, 12:02 AM

Post #2 of 5 (20 views)
Permalink

[perl #3330] Magic increment avoids warning unexpectedly [In reply to]

On Sun Aug 04 23:48:42 2013, tonyc wrote:
> On Tue Jul 09 22:30:15 2013, tonyc wrote:
> > On Sat Jul 06 12:46:41 2013, perl.p5p [at] rjbs wrote:
> > > Yes, I think so.
> >
> > I'll produce a better patch for it sometime.
> >
> > One problem with my patch above is it complains "Argument 'foo!' isn't
> > numeric..." but ++ accepts non-numeric arguments, so the message needs
> > to change.
>
> Here's an improved change with tests and a different message.

‘Isn’t incrementable’ sounds to me like a croak message, rather than a
warning. I think ‘Argument "A1A" treated as 0 in postincrement (++)’
would be better.

--

Father Chrysostomos

---
via perlbug: queue: perl5 status: open
https://rt.perl.org:443/rt3/Ticket/Display.html?id=3330

perlbug-followup at perl

Aug 5, 2013, 5:22 PM

Post #3 of 5 (15 views)
Permalink

[perl #3330] Magic increment avoids warning unexpectedly [In reply to]

On Mon Aug 05 00:02:29 2013, sprout wrote:
> On Sun Aug 04 23:48:42 2013, tonyc wrote:
> > Here's an improved change with tests and a different message.
>
> ‘Isn’t incrementable’ sounds to me like a croak message, rather than a
> warning. I think ‘Argument "A1A" treated as 0 in postincrement (++)’
> would be better.

I (mostly) prefer your message.

I used just "increment" because sometimes the optimization (I assume)
changes a post-increment into a pre-increment:

tony [at] mar:.../git/perl$ ./perl -Ilib -MO=Deparse -e '$x++'
++$x;

so OP_DESC(PL_op) returns the wrong value.

An extra improvement might be to report the variable as undefined value
warnings do, I might add that.

Tony

---
via perlbug: queue: perl5 status: open
https://rt.perl.org:443/rt3/Ticket/Display.html?id=3330

perlbug-followup at perl

Aug 11, 2013, 10:07 PM

Post #4 of 5 (9 views)
Permalink

[perl #3330] Magic increment avoids warning unexpectedly [In reply to]

On Mon Aug 05 17:22:09 2013, tonyc wrote:
> On Mon Aug 05 00:02:29 2013, sprout wrote:
> > On Sun Aug 04 23:48:42 2013, tonyc wrote:
> > > Here's an improved change with tests and a different message.
> >
> > ‘Isn’t incrementable’ sounds to me like a croak message, rather than a
> > warning. I think ‘Argument "A1A" treated as 0 in postincrement (++)’
> > would be better.
>
> I (mostly) prefer your message.
>
> I used just "increment" because sometimes the optimization (I assume)
> changes a post-increment into a pre-increment:
>
> tony [at] mar:.../git/perl$ ./perl -Ilib -MO=Deparse -e '$x++'
> ++$x;
>
> so OP_DESC(PL_op) returns the wrong value.
>
> An extra improvement might be to report the variable as undefined value
> warnings do, I might add that.

I've pushed a version of this to blead with the rephrased warning.

I experiemented with using find_uninit_var() to report the variable (see
below), which was fairly trivial, but we don't seem to be using this
elsewhere for reporting variable names in other places (like reporting
symbolic reference errors.)

Is that more due to lack or tuits or an attempt to avoid an explosion of
diagnostic messages?

Or is it a bad idea for other reasons?

Tony

--- a/sv.c
+++ b/sv.c
@@ -1822,13 +1822,20 @@ S_not_incrementable(pTHX_ SV *const sv) {
dVAR;
char tmpbuf[64];
const char *pv;
+ SV *varname;

PERL_ARGS_ASSERT_NOT_INCREMENTABLE;

pv = sv_display(sv, tmpbuf, sizeof(tmpbuf));

- Perl_warner(aTHX_ packWARN(WARN_NUMERIC),
- "Argument \"%s\" treated as 0 in increment (++)", pv);
+ if (PL_op && (varname = find_uninit_var(PL_op, sv, 0)) != NULL) {
+ Perl_warner(aTHX_ packWARN(WARN_NUMERIC),
+ "Argument \"%s\" in %"SVf" treated as 0 in
increment (++)"
+ }
+ else {
+ Perl_warner(aTHX_ packWARN(WARN_NUMERIC),
+ "Argument \"%s\" treated as 0 in increment (++)", pv);
+ }
}

---
via perlbug: queue: perl5 status: open
https://rt.perl.org:443/rt3/Ticket/Display.html?id=3330

perlbug-followup at perl

Aug 11, 2013, 11:48 PM

Post #5 of 5 (9 views)
Permalink

[perl #3330] Magic increment avoids warning unexpectedly [In reply to]

On Sun Aug 11 22:07:13 2013, tonyc wrote:
> On Mon Aug 05 17:22:09 2013, tonyc wrote:
> > On Mon Aug 05 00:02:29 2013, sprout wrote:
> > > On Sun Aug 04 23:48:42 2013, tonyc wrote:
> > > > Here's an improved change with tests and a different message.
> > >
> > > ‘Isn’t incrementable’ sounds to me like a croak message, rather than a
> > > warning. I think ‘Argument "A1A" treated as 0 in postincrement (++)’
> > > would be better.
> >
> > I (mostly) prefer your message.
> >
> > I used just "increment" because sometimes the optimization (I assume)
> > changes a post-increment into a pre-increment:
> >
> > tony [at] mar:.../git/perl$ ./perl -Ilib -MO=Deparse -e '$x++'
> > ++$x;
> >
> > so OP_DESC(PL_op) returns the wrong value.
> >
> > An extra improvement might be to report the variable as undefined value
> > warnings do, I might add that.
>
> I've pushed a version of this to blead with the rephrased warning.
>
> I experiemented with using find_uninit_var() to report the variable (see
> below), which was fairly trivial, but we don't seem to be using this
> elsewhere for reporting variable names in other places (like reporting
> symbolic reference errors.)
>
> Is that more due to lack or tuits or an attempt to avoid an explosion of
> diagnostic messages?
>
> Or is it a bad idea for other reasons?

I don’t think it’s a bad idea in principle, but find_uninit_var would
need to be reworked a little, to avoid false positives.

Currently it assumes that if a binop has a defined constant argument
then the other argument must be the source of the undefined warning.
(I.e., if $x+2 gives a warning, $x must be the culprit.) I have not
been able to trigger a false positive, as ++ takes a scalar lvalue
expression, but still it seems the wrong way to implement it.

I would suggest refactoring parts of find_uninit_var or ck_length or
bind_match into an op_varname function.

--

Father Chrysostomos

---
via perlbug: queue: perl5 status: open
https://rt.perl.org:443/rt3/Ticket/Display.html?id=3330

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact Gossamer Threads