code-review/2009-07-22
object a1834a8ee289b1433049c65a0b29ec7c74ed44db
authorDavid Golden <dagolden@cpan.org>
Thu, 23 Jul 2009 01:39:13 +0000 (21:39 -0400)
Code Review from IRC

On #corehackers:
17:56  jjore> Did you mean to drop all the MAD support?
17:56  nperez> yeah, i was noticing that too
17:57  jjore> I thought MAD was rotting but I wouldn't remove it.
18:26  xdg> jjore, I dropped it because I didn't understand it and someone
            here said to take it out
18:46  jdb> xdg: Larry said the MAD stuff should not be dropped, even if it
            bitrots a bit: http://use.perl.org/~chromatic/journal/39125
18:51  xdg> jdb, noted.  thanks.
18:54  xdg> reading that, I'll call what I did "bitrot".  Since I don't
            understand MAD, I don't know how to make it do the right thing
            as I change the code around it, so it's out.  That's different
            than someone ripping all the MAD stuff out for the sake of
            eliminating MAD
18:55  jdb> Maybe just leave the code in there, commented out with a short
            explanation
18:55  jdb> That's easier than tracking down the missing bit in git later.
18:55  jdb> At least it gives someone who understands the MAD stuff a better
            chance to see what might be missing

On #p5p
18:26 @jjore> xdg, in http://github.com/dagolden/perl/commit/5fdce45a7889979e70453fa36d2dd3aa1c73d808,
              I'm not sure that package names can't have nulls in them.
18:26 +dipsy> urgh. long url. Try http://tinyurl.com/nus6p9
18:26 @jjore> though, never when parsed.
18:27 @xdg> jjore, that's a version number, which can't be null
18:28 @jjore> Ah. Long ago I asked on perl5-porters@ for Larry's definition of
              each of the MAD tokens. You could use that.
18:28 @jjore> but I meant the package, not the version.
18:29 @jjore> Null handling in packages is problematic and not supported tho,
              I think.
18:29 @jjore> Once, I said bless([], "\0") to bless at the '' package but
              not get the ''->'main' conversion
18:29 @xdg> I'm not sure I understand the problem.
18:30 @jjore> *{"Devel::\0::Hi::VERSION"} = ...
18:30 @xdg> I think if PL_curstname is "\0", then package_version ends up
            trying to set "$::VERSION"
18:32 @jjore> No, ${"\0::VERSION"}
18:33 @jjore> I feel like I'm bike-shedding though.
18:36 @jjore> If the current source code is utf8, presumably $VERSION
              will be too
18:36 @jjore> and did you drop support for unversioned packages?
18:37 @jjore> maybe I'm not reading the grammar right.
18:48 @xdg> unversioned packages are just fine.  $VERSION will be whatever
            scan_version() parses, which is what "use MODULE VERSION" does,
            so if that's broken for utf8, it's broken for both
18:48 @jjore> fascinating.
18:48 @jjore> I wonder if it is.
18:49 @jjore> or it's an interesting corner to poke at sometime.
18:49 @xdg> Since I'm doing sv_catpv(), I would think that if the *PV of
            PL_curstnam is "\0", that it concatenates like the empty string
18:50 @xdg> Oh, I see.  *PV is actually "\0\0" with LEN 1
18:51 @xdg> I'd suggest fixing that by banning \0 as a package name.
            It's stupid in the first place.