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

"Useless use of concatenation" warning not triggered by multiple concatenations. #3990

Closed
p5pRT opened this issue May 15, 2001 · 13 comments
Closed

Comments

@p5pRT
Copy link
Collaborator

@p5pRT p5pRT commented May 15, 2001

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

Searchable as RT6997$

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented May 15, 2001

From pnewton@gmx.de

$ ./perl -w -e '($a,$b)=(42)x2; $a . $b'
Useless use of concatenation (.) or string in void context at -e line 1.
$ ./perl -w -e '($a,$b,$c)=(42)x3; $a . $b . $c'
(nothing)

To me, the second concatenation is also in void context. Even if you
parenthesize it, (($a . $b) . $c), the first concatenation is in scalar
context because its value is used for the second concatenation -- but
the second concatenation, at least, is in void context, isn't it?

category=core
severity=low

Cheers,
Philip

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 16, 2016

From @wolfsage

On Mon May 14 21​:22​:06 2001, pnewton@​gmx.de wrote​:

$ ./perl -w -e '($a,$b)=(42)x2; $a . $b'
Useless use of concatenation (.) or string in void context at -e line 1.
$ ./perl -w -e '($a,$b,$c)=(42)x3; $a . $b . $c'
(nothing)

To me, the second concatenation is also in void context.

Agreed.

This seems to stem from some old code that I'm not sure is necessary any more.

Perl_ck_concat does​:

  if (kid->op_type == OP_CONCAT && !(kid->op_private & OPpTARGET_MY) &&
  !(kUNOP->op_first->op_flags & OPf_MOD))
  o->op_flags |= OPf_STACKED;

But why does it do this?

Removing Perl_ck_concat makes $a . $b . $c warn properly, and doesn't break the test suite (legit, right?).

Compare (pre-patch) how multiple add ops are constructed versus multiple concat ops​:

  mhorsfall@​tworivers​:~/p5/perl$ perl -MO=Concise,-exec -e '$x + $a'
  1 <0> enter
  2 <;> nextstate(main 1 -e​:1) v​:{
  3 <#> gvsv[*x] s
  4 <#> gvsv[*a] s
  5 <2> add[t3] vK/2
  6 <@​> leave[1 ref] vKP/REFC
  -e syntax OK
mhorsfall@​tworivers​:~/p5/perl$ perl -MO=Concise,-exec -e '$x + $a + $a'
  1 <0> enter
  2 <;> nextstate(main 1 -e​:1) v​:{
  3 <#> gvsv[*x] s
  4 <#> gvsv[*a] s
  5 <2> add[t3] sK/2
  6 <#> gvsv[*a] s
  7 <2> add[t5] vK/2
  8 <@​> leave[1 ref] vKP/REFC
  -e syntax OK

  mhorsfall@​tworivers​:~/p5/perl$ perl -MO=Concise,-exec -e '$x . $a'
  1 <0> enter
  2 <;> nextstate(main 1 -e​:1) v​:{
  3 <#> gvsv[*x] s
  4 <#> gvsv[*a] s
  5 <2> concat[t3] vK/2
  6 <@​> leave[1 ref] vKP/REFC
  -e syntax OK
  mhorsfall@​tworivers​:~/p5/perl$ perl -MO=Concise,-exec -e '$x . $a . $a'
  1 <0> enter
  2 <;> nextstate(main 1 -e​:1) v​:{
  3 <#> gvsv[*x] s
  4 <#> gvsv[*a] s
  5 <2> concat[t3] sK/2
  6 <#> gvsv[*a] s
  7 <2> concat[t5] vKS/2 <-- OPf_STACKED
  8 <@​> leave[1 ref] vKP/REFC

With Perl_ck_concat, the final concat is modified to be OPf_STACKED, which causes the void warning to not trigger.

Is . fundamentally different than + in some way I don't know, or is this just old code that's no longer needed?

Possible patch attached.

-- Matthew Horsfall (alh)

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 16, 2016

From @wolfsage

0001-perl-6997-ck_concat-is-setting-OPf_STACKED-when-it-s.patch
From 17b48bfcbeedf308e8ff26037de7dd8de0eaae4f Mon Sep 17 00:00:00 2001
From: Matthew Horsfall <wolfsage@gmail.com>
Date: Sat, 16 Jan 2016 16:29:47 -0500
Subject: [PATCH] [perl #6997] - ck_concat is setting OPf_STACKED when it
 shouldn't.

In fact, why do we even have a ck_concat? It was introduced with
perl 5.0, but seems to serve no purpose now?
---
 embed.h             |  1 -
 op.c                | 14 --------------
 opcode.h            |  2 +-
 proto.h             |  5 -----
 regen/opcodes       |  2 +-
 t/lib/warnings/2use |  2 ++
 6 files changed, 4 insertions(+), 22 deletions(-)

diff --git a/embed.h b/embed.h
index 73c02d2..63dff89 100644
--- a/embed.h
+++ b/embed.h
@@ -1110,7 +1110,6 @@
 #define ck_backtick(a)		Perl_ck_backtick(aTHX_ a)
 #define ck_bitop(a)		Perl_ck_bitop(aTHX_ a)
 #define ck_cmp(a)		Perl_ck_cmp(aTHX_ a)
-#define ck_concat(a)		Perl_ck_concat(aTHX_ a)
 #define ck_defined(a)		Perl_ck_defined(aTHX_ a)
 #define ck_delete(a)		Perl_ck_delete(aTHX_ a)
 #define ck_each(a)		Perl_ck_each(aTHX_ a)
diff --git a/op.c b/op.c
index ee31adc..a481001 100644
--- a/op.c
+++ b/op.c
@@ -9386,20 +9386,6 @@ Perl_ck_cmp(pTHX_ OP *o)
 }
 
 OP *
-Perl_ck_concat(pTHX_ OP *o)
-{
-    const OP * const kid = cUNOPo->op_first;
-
-    PERL_ARGS_ASSERT_CK_CONCAT;
-    PERL_UNUSED_CONTEXT;
-
-    if (kid->op_type == OP_CONCAT && !(kid->op_private & OPpTARGET_MY) &&
-	    !(kUNOP->op_first->op_flags & OPf_MOD))
-        o->op_flags |= OPf_STACKED;
-    return o;
-}
-
-OP *
 Perl_ck_spair(pTHX_ OP *o)
 {
     dVAR;
diff --git a/opcode.h b/opcode.h
index e711e65..0747f91 100644
--- a/opcode.h
+++ b/opcode.h
@@ -1440,7 +1440,7 @@ EXT Perl_check_t PL_check[] /* or perlvars.h */
 	Perl_ck_null,		/* i_add */
 	Perl_ck_null,		/* subtract */
 	Perl_ck_null,		/* i_subtract */
-	Perl_ck_concat,		/* concat */
+	Perl_ck_null,		/* concat */
 	Perl_ck_stringify,	/* stringify */
 	Perl_ck_bitop,		/* left_shift */
 	Perl_ck_bitop,		/* right_shift */
diff --git a/proto.h b/proto.h
index 1bbdace..188a915 100644
--- a/proto.h
+++ b/proto.h
@@ -288,11 +288,6 @@ PERL_CALLCONV OP *	Perl_ck_cmp(pTHX_ OP *o)
 #define PERL_ARGS_ASSERT_CK_CMP	\
 	assert(o)
 
-PERL_CALLCONV OP *	Perl_ck_concat(pTHX_ OP *o)
-			__attribute__warn_unused_result__;
-#define PERL_ARGS_ASSERT_CK_CONCAT	\
-	assert(o)
-
 PERL_CALLCONV OP *	Perl_ck_defined(pTHX_ OP *o)
 			__attribute__warn_unused_result__;
 #define PERL_ARGS_ASSERT_CK_DEFINED	\
diff --git a/regen/opcodes b/regen/opcodes
index 9ea0753..f179cef 100644
--- a/regen/opcodes
+++ b/regen/opcodes
@@ -133,7 +133,7 @@ add		addition (+)		ck_null		IfsT2	S S
 i_add		integer addition (+)	ck_null		ifsT2	S S
 subtract	subtraction (-)		ck_null		IfsT2	S S
 i_subtract	integer subtraction (-)	ck_null		ifsT2	S S
-concat		concatenation (.) or string	ck_concat	fsT2	S S
+concat		concatenation (.) or string	ck_null	fsT2	S S
 stringify	string			ck_stringify	fsT@	S
 
 left_shift	left bitshift (<<)	ck_bitop	fsT2	S S
diff --git a/t/lib/warnings/2use b/t/lib/warnings/2use
index c0d203a..c4b724e 100644
--- a/t/lib/warnings/2use
+++ b/t/lib/warnings/2use
@@ -75,8 +75,10 @@ Reversed += operator at - line 3.
 -w
 no warnings 'reserved' ;
 foo.bar;
+foo.bar.baz;
 EXPECT
 Useless use of concatenation (.) or string in void context at - line 3.
+Useless use of concatenation (.) or string in void context at - line 4.
 ########
 
 --FILE-- abc
-- 
1.9.1

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 16, 2016

From [Unknown Contact. See original ticket]

On Mon May 14 21​:22​:06 2001, pnewton@​gmx.de wrote​:

$ ./perl -w -e '($a,$b)=(42)x2; $a . $b'
Useless use of concatenation (.) or string in void context at -e line 1.
$ ./perl -w -e '($a,$b,$c)=(42)x3; $a . $b . $c'
(nothing)

To me, the second concatenation is also in void context.

Agreed.

This seems to stem from some old code that I'm not sure is necessary any more.

Perl_ck_concat does​:

  if (kid->op_type == OP_CONCAT && !(kid->op_private & OPpTARGET_MY) &&
  !(kUNOP->op_first->op_flags & OPf_MOD))
  o->op_flags |= OPf_STACKED;

But why does it do this?

Removing Perl_ck_concat makes $a . $b . $c warn properly, and doesn't break the test suite (legit, right?).

Compare (pre-patch) how multiple add ops are constructed versus multiple concat ops​:

  mhorsfall@​tworivers​:~/p5/perl$ perl -MO=Concise,-exec -e '$x + $a'
  1 <0> enter
  2 <;> nextstate(main 1 -e​:1) v​:{
  3 <#> gvsv[*x] s
  4 <#> gvsv[*a] s
  5 <2> add[t3] vK/2
  6 <@​> leave[1 ref] vKP/REFC
  -e syntax OK
mhorsfall@​tworivers​:~/p5/perl$ perl -MO=Concise,-exec -e '$x + $a + $a'
  1 <0> enter
  2 <;> nextstate(main 1 -e​:1) v​:{
  3 <#> gvsv[*x] s
  4 <#> gvsv[*a] s
  5 <2> add[t3] sK/2
  6 <#> gvsv[*a] s
  7 <2> add[t5] vK/2
  8 <@​> leave[1 ref] vKP/REFC
  -e syntax OK

  mhorsfall@​tworivers​:~/p5/perl$ perl -MO=Concise,-exec -e '$x . $a'
  1 <0> enter
  2 <;> nextstate(main 1 -e​:1) v​:{
  3 <#> gvsv[*x] s
  4 <#> gvsv[*a] s
  5 <2> concat[t3] vK/2
  6 <@​> leave[1 ref] vKP/REFC
  -e syntax OK
  mhorsfall@​tworivers​:~/p5/perl$ perl -MO=Concise,-exec -e '$x . $a . $a'
  1 <0> enter
  2 <;> nextstate(main 1 -e​:1) v​:{
  3 <#> gvsv[*x] s
  4 <#> gvsv[*a] s
  5 <2> concat[t3] sK/2
  6 <#> gvsv[*a] s
  7 <2> concat[t5] vKS/2 <-- OPf_STACKED
  8 <@​> leave[1 ref] vKP/REFC

With Perl_ck_concat, the final concat is modified to be OPf_STACKED, which causes the void warning to not trigger.

Is . fundamentally different than + in some way I don't know, or is this just old code that's no longer needed?

Possible patch attached.

-- Matthew Horsfall (alh)

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jul 4, 2016

From @dcollinsn

Matt's patch works, except for the test, which no longer applies. The test he was patching has since been changed. the attached patch adds a test, which fails in blead and passes when applying Matt's patch.

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jul 4, 2016

From @dcollinsn

0002-RT-6997-Test-for-warnings-for-a.-b.-c.patch
From 315ed2c3ebb4fbe8a649d6ed8df084fd7a97c644 Mon Sep 17 00:00:00 2001
From: Dan Collins <dcollinsn@gmail.com>
Date: Mon, 4 Jul 2016 19:11:05 -0400
Subject: [PATCH] [RT #6997] Test for warnings for $a.$b.$c

---
 t/lib/warnings/2use | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/t/lib/warnings/2use b/t/lib/warnings/2use
index 4e10d4b..dc7c8c7 100644
--- a/t/lib/warnings/2use
+++ b/t/lib/warnings/2use
@@ -377,3 +377,13 @@ $#;
 EXPECT
 $* is no longer supported at - line 3.
 $# is no longer supported at - line 5.
+########
+
+# Useless use of concatenation should appear for any number of args
+use warnings;
+($a, $b, $c) = (42)x3;
+$a.$b;
+$a.$b.$c;
+EXPECT
+Useless use of concatenation (.) or string in void context at - line 5.
+Useless use of concatenation (.) or string in void context at - line 6.
-- 
2.8.1

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jul 4, 2016

From [Unknown Contact. See original ticket]

Matt's patch works, except for the test, which no longer applies. The test he was patching has since been changed. the attached patch adds a test, which fails in blead and passes when applying Matt's patch.

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 29, 2016

From @iabyn

On Sat, Jan 16, 2016 at 01​:40​:08PM -0800, Matthew Horsfall via RT wrote​:

On Mon May 14 21​:22​:06 2001, pnewton@​gmx.de wrote​:

$ ./perl -w -e '($a,$b)=(42)x2; $a . $b'
Useless use of concatenation (.) or string in void context at -e line 1.
$ ./perl -w -e '($a,$b,$c)=(42)x3; $a . $b . $c'
(nothing)

To me, the second concatenation is also in void context.

Agreed.

This seems to stem from some old code that I'm not sure is necessary any more.

Perl_ck_concat does​:

if \(kid\->op\_type == OP\_CONCAT && \!\(kid\->op\_private & OPpTARGET\_MY\) &&
        \!\(kUNOP\->op\_first\->op\_flags & OPf\_MOD\)\)
    o\->op\_flags |= OPf\_STACKED;

But why does it do this?

Removing Perl_ck_concat makes $a . $b . $c warn properly, and doesn't break the test suite (legit, right?).

It's an optimisation, so I suggest not removing it.

The (($a . $b) . $c) is being compiled as (essentially)

  (($a . $b) .= $c)

i.e. the temporary (padtmp) result of the first concat is just reused and
appended to, rather than using a second temporary and copying both strings
to it.

As an aside, I've occasionally thought that we need an OP_CONCATLIST op,
where rpeep() converts a nested tree of concat ops into a (PUSHMARK, arg,
arg, ..., CONCATLIST) sequence, where the CONCATLIST op is a bit like
join('', ....) (but more efficient).

--
Overhead, without any fuss, the stars were going out.
  -- Arthur C Clarke

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 29, 2016

From @cpansprout

On Thu Sep 29 02​:18​:43 2016, davem wrote​:

As an aside, I've occasionally thought that we need an OP_CONCATLIST
op,
where rpeep() converts a nested tree of concat ops into a (PUSHMARK,
arg,
arg, ..., CONCATLIST) sequence, where the CONCATLIST op is a bit like
join('', ....) (but more efficient).

If it turns out to be more efficient, then join '', ... should be optimised into CONCATLIST.

--

Father Chrysostomos

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Dec 14, 2017

From zefram@fysh.org

Fixed in commit 3d03338.

-zefram

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Dec 17, 2017

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

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 23, 2018

From @khwilliamson

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

With the release yesterday of Perl 5.28.0, this and 185 other issues have been
resolved.

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

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

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 23, 2018

@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
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant
You can’t perform that action at this time.