[perl #115736] newATTRSUB_flags has an undocumented bitfield/flags and misc about newATTRSUB

[bulk 88]

# New Ticket Created by bulk 88
# Please include the string: [perl #115736]
# in the subject line of all future correspondence about this issue.
# <URL: https://rt.perl.org:443/rt3/Ticket/Display.html?id=115736 >


This is a bug report for perl from bul...@hotmail.com,
generated with the help of perlbug 1.39 running under perl 5.17.6.


-----------------------------------------------------------------
[Please describe your issue here]

In
http://perl5.git.perl.org/perl.git/commitdiff/7e68c38b607a044ee5879e316bb8a7347284ec8e
Father Chrysostomos added a flags parameter to newATTRSUB. The flags
parameter is undocumented and is using plain numeric flags
http://perl5.git.perl.org/perl.git/blob/09c759566a430d381e168ae0530363f206c90715:/op.c#l7325
. Can I request that flags param either be renamed to a bool or t/f
param parameter that described what is does, or a macro define be made
to give that 1 a readable name?

I'd like to do some cleanup of newATTRSUB_flags but that line is
bothering me since I might (I don't have a full plan right now in my
head for what I'll change) have to write (flags & 1) more times than it
already is written.

I am leaning towards a t/f than a "flags" since there is only 1 flag
right now (see end for more detailed rambling) and since
newATTRSUB_flags isn't public, it can be renamed or upgraded back to
_flags from _bool or _x (x=extended) (guessing names) in the future if
more flags will need to be added, but I dont know the future or have a
clear cut implementation (names, or t/f vs flags, or future flags) in my
head.

Here are the callers of the "old" newATTRSUB in blead
Perl_newSUB
Perl_pmruntime (2 calls)
Perl_utilize
Perl_newANONATTRSUB
Perl_yyparse

As a side discussion to consider, I might (not decided/more research)
replace their calls to newATTRSUB with newATTRSUB_flags (or whatever it
is renamed to if it looses its "flags") to avoid indirection, leaving
newATTRSUB strictly as an export or converting newATTRSUB to a macro
(CPAN grep gives me ~4 distros that use newATTRSUB).

Since Father Chrysostomos authored that commit, I am CCing him.

Skip this paragraph if you don't want the details, 2nd reason for me
leaning towards a t/f is "test stack+flagsoffset,
reg_already_containing_zero; jump_zero" is smaller than "test /* or and
*/ stack+flagsoffset, 0x01 /*or 0x00000001 depending on platform */
jump_zero;" or using up another stack var for whats already on the stack
(on my 32bit machine uses 92 bytes of C stack (not counting the param
list), 49 of those bytes are used by this
http://perl5.git.perl.org/perl.git/blob/09c759566a430d381e168ae0530363f206c90715:/op.c#l7382
git blamed to
http://perl5.git.perl.org/perl.git/commitdiff/e1ec3a884f8d8c64eb7e391b2a363f47cbeed570
, 49 aligned to 4 is 52, (92-52) = 40/4, leaving 10 non-registered C
pointer sized autos in newATTRSUB_flags).

[Please do not change anything below this line]
-----------------------------------------------------------------
---
Flags:
category=core
severity=low
---
Site configuration information for perl 5.17.6:

Configured by Owner at Tue Nov 13 02:44:40 2012.

Summary of my perl5 (revision 5 version 17 subversion 6 patch blead
2012-11-12.23:49:19 a36462570c37e1b6544fc8746e99db3d683e2ac1
v5.17.5-458-ga364625) configuration:
Snapshot of: a36462570c37e1b6544fc8746e99db3d683e2ac1
Platform:
osname=MSWin32, osvers=5.1, archname=MSWin32-x86-multi-thread
uname=''
config_args='undef'
hint=recommended, useposix=true, d_sigaction=undef
useithreads=define, usemultiplicity=define
useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
use64bitint=undef, use64bitall=undef, uselongdouble=undef
usemymalloc=n, bincompat5005=undef
Compiler:
cc='cl', ccflags ='-nologo -GF -W3 -MD -Zi -DNDEBUG -Od -G7 -GL
-arch:SSE2 -DWIN32 -D_CONSOLE -DNO_STRICT -DPERL_TEXTMODE_SCRIPTS
-DPERL_IMPLICIT_CONTEXT -DPERL_IMPLICIT_SYS -DUSE_PERLIO
-D_USE_32BIT_TIME_T',
optimize='-MD -Zi -DNDEBUG -Od -G7 -GL -arch:SSE2',
cppflags='-DWIN32'
ccversion='13.10.6030', gccversion='', gccosandvers=''
intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=1234
d_longlong=undef, longlongsize=8, d_longdbl=define, longdblsize=8
ivtype='long', ivsize=4, nvtype='double', nvsize=8, Off_t='__int64',
lseeksize=8
alignbytes=8, prototype=define
Linker and Libraries:
ld='link', ldflags ='-nologo -nodefaultlib -debug -opt:ref,icf
-ltcg -libpath:"c:\perl517\lib\CORE" -machine:x86'
libpth="C:\Program Files\Microsoft Visual Studio .NET 2003\VC7\lib"
libs=oldnames.lib kernel32.lib user32.lib gdi32.lib winspool.lib
comdlg32.lib advapi32.lib shell32.lib ole32.lib oleaut32.lib
netapi32.lib uuid.lib ws2_32.lib mpr.lib winmm.lib version.lib
odbc32.lib odbccp32.lib comctl32.lib msvcrt.lib
perllibs=oldnames.lib kernel32.lib user32.lib gdi32.lib
winspool.lib comdlg32.lib advapi32.lib shell32.lib ole32.lib
oleaut32.lib netapi32.lib uuid.lib ws2_32.lib mpr.lib winmm.lib
version.lib odbc32.lib odbccp32.lib comctl32.lib msvcrt.lib
libc=msvcrt.lib, so=dll, useshrplib=true, libperl=perl517.lib
gnulibc_version=''
Dynamic Linking:
dlsrc=dl_win32.xs, dlext=dll, d_dlsymun=undef, ccdlflags=' '
cccdlflags=' ', lddlflags='-dll -nologo -nodefaultlib -debug
-opt:ref,icf -ltcg -libpath:"c:\perl517\lib\CORE" -machine:x86'

Locally applied patches:


---
@INC for perl 5.17.6:
C:/perl517/site/lib
C:/perl517/lib
.

---
Environment for perl 5.17.6:
HOME (unset)
LANG (unset)
LANGUAGE (unset)
LD_LIBRARY_PATH (unset)
LOGDIR (unset)
PATH=C:\perl517\bin;C:\Program Files\Microsoft Visual Studio .NET
2003\Common7\IDE;C:\Program Files\Microsoft Visual Studio .NET
2003\VC7\BIN;C:\Program Files\Microsoft Visual Studio .NET
2003\Common7\Tools;C:\Program Files\Microsoft Visual Studio .NET
2003\Common7\Tools\bin\prerelease;C:\WINDOWS\system32;C:\WINDOWS;C:\WINDOWS\system32\wbem;
PERL_BADLANG (unset)
SHELL (unset)

[Father Chrysostomos via RT]

On Wed Nov 14 18:33:58 2012, bulk88 wrote:
> This is a bug report for perl from bul...@hotmail.com,
> generated with the help of perlbug 1.39 running under perl 5.17.6.
>
>
> -----------------------------------------------------------------
> [Please describe your issue here]
>
> In
>
http://perl5.git.perl.org/perl.git/commitdiff/7e68c38b607a044ee5879e316bb8a7347284ec8e
> Father Chrysostomos added a flags parameter to newATTRSUB. The flags
> parameter is undocumented and is using plain numeric flags
>
http://perl5.git.perl.org/perl.git/blob/09c759566a430d381e168ae0530363f206c90715:/op.c#l7325
> . Can I request that flags param either be renamed to a bool or t/f
> param parameter that described what is does, or a macro define be made
> to give that 1 a readable name?
>
> I'd like to do some cleanup of newATTRSUB_flags but that line is
> bothering me since I might (I don't have a full plan right now in my
> head for what I'll change) have to write (flags & 1) more times than
> it
> already is written.

The only reason for that flags parameter is that, while newATTRSUB’s
interface might be perfect for the parser, it is extremely unhelpful if
you already have a GV you want to add the new sub to.

So I just did the quickest thing.

I have tried about three times to break newATTRSUB into smaller reusable
subs so that gv.c:S_maybe_add_coresub doesn’t have to go through
newATTRSUB at all, but with no success. (My attempts just made things
more complicated.) That would still be a good thing, if possible, to
avoid code duplication between newATTRSUB and newMYSUB.

>
> I am leaning towards a t/f than a "flags" since there is only 1 flag
> right now (see end for more detailed rambling) and since
> newATTRSUB_flags isn't public, it can be renamed or upgraded back to
> _flags from _bool or _x (x=extended) (guessing names) in the future
> if
> more flags will need to be added, but I dont know the future or have a
> clear cut implementation (names, or t/f vs flags, or future flags) in
> my
> head.

That’s fine with me.

>
> Here are the callers of the "old" newATTRSUB in blead
> Perl_newSUB
> Perl_pmruntime (2 calls)
> Perl_utilize
> Perl_newANONATTRSUB
> Perl_yyparse
>
> As a side discussion to consider, I might (not decided/more research)
> replace their calls to newATTRSUB with newATTRSUB_flags (or whatever
> it
> is renamed to if it looses its "flags") to avoid indirection, leaving
> newATTRSUB strictly as an export or converting newATTRSUB to a macro
> (CPAN grep gives me ~4 distros that use newATTRSUB).

If you want to remove the extra sub call (gcc optimises it away, BTW), I
would suggest leaving those callers alone and making newATTRSUB a macro.

>
> Since Father Chrysostomos authored that commit, I am CCing him.

No need to do that, as I see almost everything that goes through RT.

--

Father Chrysostomos


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

[James E Keenan via RT]

bulk88, Father C: Can we get an update on the status of this ticket?

Thank you very much.
Jim Keenan

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

[Father Chrysostomos]

Jim Keenan asked:

> bulk88, Father C: Can we get an update on the status of this ticket?

This falls under the "patches welcome" (do they?) category.

[bulk88 via RT]

On Fri Dec 13 16:29:58 2013, jkeenan wrote:
> bulk88, Father C: Can we get an update on the status of this ticket?
>

> Thank you very much.
> Jim Keenan

The param is still undocumented. Some of the misc clean up I mentioned was done in http://perl5.git.perl.org/perl.git/commit/eed484f95050ad51c720521f68c6341a14bf5638?f=op.c . Since FC says it can be turned into a bool and he has no plans of adding more flags, my plan would be newATTRSUB becomes a macro to newATTRSUB_x, and newATTRSUB_flags is renamed to private api newATTRSUB_x. U32 flags param will be renamed to bool o_is_gv. It will be a while before I get around to writing a patch but it is on my todo list.

--
bulk88 ~ bulk88 at hotmail.com

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

[bulk88 via RT]

On Fri Dec 13 16:53:58 2013, perl5-...@perl.org wrote:
>
> This falls under the "patches welcome" (do they?) category.
>

Patch attached.

--
bulk88 ~ bulk88 at hotmail.com

[Father Chrysostomos via RT]

On Sun Dec 22 23:13:45 2013, bulk88 wrote:
> On Fri Dec 13 16:53:58 2013, perl5-...@perl.org wrote:
> >

> > This falls under the "patches welcome" (do they?) category.
> >
>

> Patch attached.
>

Thank you. Applied as e8f91c91cc7.

--

Father Chrysostomos