[perl #76402] [PATCH] Locale::Maketext One Sided Lexicons
Todd Rinaldo
# Please include the string: [perl #76402]
# in the subject line of all future correspondence about this issue.
# <URL: http://rt.perl.org/rt3/Ticket/Display.html?id=76402 >
This is a bug report for perl from to...@cpanel.net,
generated with the help of perlbug 1.39 running under perl 5.12.1.
-----------------------------------------------------------------
[Please describe your issue here]
This patch with tests provides One Sided Lexicons
Setting $Onesided = 1 in your package treats the class's %Lexicon as one sided.
What that means is if the hash's keys and values will be the same (IE your main
Lexicon) you can specify it in the key only and leave the value blank.
The advantages are a smaller file, less prone to mistyping or mispasting, and most
important of all someone translating it can simply copy it into their module and
enter their translation instead of having to remove the value first.
[Please do not change anything below this line]
-----------------------------------------------------------------
---
Flags:
category=library
severity=low
module=Locale::Maketext
---
Site configuration information for perl 5.12.1:
Configured by cPanel at Mon Jun 7 11:16:28 CDT 2010.
Summary of my perl5 (revision 5 version 12 subversion 1) configuration:
Platform:
osname=linux, osvers=2.6.18-194.3.1.el5, archname=x86_64-linux
uname='linux rpmb-centos-50-64bit 2.6.18-194.3.1.el5 #1 smp thu may 13 13:08:30 edt 2010 x86_64 x86_64 x86_64 gnulinux '
config_args='-des -Darchname=x86_64-linux -Dcc=/usr/local/cpanel/bin/gcc -Dcpp=/usr/local/cpanel/bin/gcc -E -DDEBUGGING=none -Doptimize=-Os -Dusemymalloc=y -Duseshrplib=true -Duselargefiles=yes -Duseposix=true -Dhint=recommended -Duseperlio=yes -Dccflags=-I/usr/local/cpanel/include -L/usr/local/cpanel/lib64 -Wl,-rpath -Wl,/usr/local/cpanel/lib64 -Dcppflags=-I/usr/local/cpanel/include -L/usr/local/cpanel/lib64 -Dldflags=-Wl,-rpath -Wl,/usr/local/cpanel/lib64 -L/usr/local/cpanel/lib64 -Dprefix=/usr/local/cpanel -Dsiteprefix=/usr/local/cpanel -Dsitebin=/usr/local/cpanel/bin -Dsitelib=/usr/local/cpanel/lib64/perl5/site_lib -Dprivlib=/usr/local/cpanel/lib64/perl5/5.12.1 -Dotherlibdirs=/var/cpanel/perl5/lib:/usr/local/cpanel/lib64/perl5/cpanel -Dman1dir=/usr/local/cpanel/share/man/man1 -Dman3dir=/usr/local/cpanel/share/man/man3 -Dsiteman1dir=/usr/local/cpanel/share/man/man1 -Dsiteman3dir=/usr/local/cpanel/share/man/man3 -Dcf_by=cPanel -Dmyhostname=localhost -Dperladmin=root@loc
alhost -Dcf_email=sup...@cpanel.net -Di_dbm=/usr/local/cpanel/include -Di_gdbm=/usr/local/cpanel/include -Di_ndbm=/usr/local/cpanel/include -Ud_dosuid -Uuserelocatableinc -Umad -Uusethreads -Uusemultiplicity -Uusesocks -Uuselongdouble -Ui_db -Aldflags=-L/usr/local/cpanel/lib64 -L/usr/lib64 -L/lib64 -lgdbm -Dlocincpth=/usr/local/cpanel/include /usr/local/include -Acflags=-fPIC -DPIC -m64 -I/usr/local/cpanel/include -Dlibpth=/usr/local/cpanel/lib64 /usr/local/lib64 /usr/local/lib /lib64 /usr/lib64 -Duse64bitint=yes -Duse64bitall=yes'
hint=recommended, useposix=true, d_sigaction=define
useithreads=undef, usemultiplicity=undef
useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
use64bitint=define, use64bitall=define, uselongdouble=undef
usemymalloc=y, bincompat5005=undef
Compiler:
cc='/usr/local/cpanel/bin/gcc', ccflags ='-I/usr/local/cpanel/include -L/usr/local/cpanel/lib64 -Wl,-rpath -Wl,/usr/local/cpanel/lib64 -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/cpanel/include -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
optimize='-Os',
cppflags='-I/usr/local/cpanel/include -L/usr/local/cpanel/lib64 -I/usr/local/cpanel/include -L/usr/local/cpanel/lib64 -Wl,-rpath -Wl,/usr/local/cpanel/lib64 -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/cpanel/include -I/usr/local/include'
ccversion='', gccversion='4.5.0', gccosandvers=''
intsize=4, longsize=8, ptrsize=8, doublesize=8, byteorder=12345678
d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16
ivtype='long', ivsize=8, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8
alignbytes=8, prototype=define
Linker and Libraries:
ld='/usr/local/cpanel/bin/gcc', ldflags ='-Wl,-rpath -Wl,/usr/local/cpanel/lib64 -L/usr/local/cpanel/lib64 -L/usr/local/cpanel/lib64 -L/usr/lib64 -L/lib64 -lgdbm -fstack-protector -L/usr/local/lib'
libpth=/usr/local/cpanel/lib64 /usr/local/lib64 /usr/local/lib /lib64 /usr/lib64
libs=-lnsl -lgdbm -ldb -ldl -lm -lcrypt -lutil -lc -lgdbm_compat
perllibs=-lnsl -ldl -lm -lcrypt -lutil -lc
libc=/lib/libc-2.5.so, so=so, useshrplib=true, libperl=libperl.so
gnulibc_version='2.5'
Dynamic Linking:
dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E -Wl,-rpath,/usr/local/cpanel/lib64/perl5/5.12.1/x86_64-linux/CORE'
cccdlflags='-fPIC', lddlflags='-shared -Os -L/usr/local/cpanel/lib64 -L/usr/lib64 -L/lib64 -L/usr/local/lib -fstack-protector'
Locally applied patches:
cPanel Patches
---
@INC for perl 5.12.1:
/usr/local/cpanel/lib64/perl5/site_lib/x86_64-linux
/usr/local/cpanel/lib64/perl5/site_lib
/usr/local/cpanel/lib64/perl5/5.12.1/x86_64-linux
/usr/local/cpanel/lib64/perl5/5.12.1
/var/cpanel/perl5/lib
/usr/local/cpanel/lib64/perl5/cpanel
.
---
Environment for perl 5.12.1:
HOME=/home/toddr
LANG=en_US.UTF-8
LANGUAGE (unset)
LD_LIBRARY_PATH (unset)
LOGDIR (unset)
PATH=/usr/local/cpanel/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/bin:/usr/X11/bin:
PERL_BADLANG (unset)
SHELL=/bin/zsh
Jesse Vincent
On Tue 6.Jul'10 at 9:43:59 -0700, Todd Rinaldo wrote:
> # New Ticket Created by Todd Rinaldo
> # Please include the string: [perl #76402]
> # in the subject line of all future correspondence about this issue.
> # <URL: http://rt.perl.org/rt3/Ticket/Display.html?id=76402 >
>
>
> This is a bug report for perl from to...@cpanel.net,
> generated with the help of perlbug 1.39 running under perl 5.12.1.
>
>
> -----------------------------------------------------------------
> [Please describe your issue here]
>
> This patch with tests provides One Sided Lexicons
>
> Setting $Onesided = 1 in your package treats the class's %Lexicon as one sided.
> What that means is if the hash's keys and values will be the same (IE your main
> Lexicon) you can specify it in the key only and leave the value blank.
>
> The advantages are a smaller file, less prone to mistyping or mispasting, and most
> important of all someone translating it can simply copy it into their module and
> enter their translation instead of having to remove the value first.
>
Locale::Maketext already has functionality to do what you want.
The _AUTO meta-entry in %Lexicon allows fallthrough to use the requested
$key as the $value. And then your base lexicon uses even _less_ memory.
The value of having your translators hand-copy the content of your
default lexicon doesn't feel like much of a win for the added
complexity.
Best,
Jesse
Todd Rinaldo
On Jul 8, 2010, at 8:57 AM, Jesse Vincent wrote:
>
>
>
> On Tue 6.Jul'10 at 9:43:59 -0700, Todd Rinaldo wrote:
>> # <URL: http://rt.perl.org/rt3/Ticket/Display.html?id=76402 >
>>
> Locale::Maketext already has functionality to do what you want.
> The _AUTO meta-entry in %Lexicon allows fallthrough to use the requested
> $key as the $value. And then your base lexicon uses even _less_ memory.
>
> The value of having your translators hand-copy the content of your
> default lexicon doesn't feel like much of a win for the added
> complexity.
>
Jesse,
Onesided-ness is a complementary feature to _AUTO, not a replacement.
When you have code that uses _AUTO, Locale fall back happens. The only way to prevent this is to inject a key with an equivalent value to prevent fallback. Sometimes unexpected defaulting might happen. This isn't something you'd use all the time, but in the cases where you do, onesided-ness is more "fat-finger safe" than duping key and value pairs.
Having key/value pairs is also useful when you have _AUTO off. In those cases, the only way have MT not break is to put in a key with an equivalent value. Again, you're more "fat-finger safe" by using one sidedness in those cases.
The added complexity only happens once when the statement is compiled, so it shouldn't be that expensive from a code path standpoint.
Thanks,
Todd Rinaldo
Jesse Vincent
On Thu, Jul 15, 2010 at 10:37:19AM -0500, Todd Rinaldo wrote:
>
> On Jul 8, 2010, at 8:57 AM, Jesse Vincent wrote:
>
> >
> Jesse,
>
> Onesided-ness is a complementary feature to _AUTO, not a replacement.
>
> When you have code that uses _AUTO, Locale fall back happens. The only way to prevent this is to inject a key with an equivalent value to prevent fallback. Sometimes unexpected defaulting might happen. This isn't something you'd use all the time, but in the cases where you do, onesided-ness is more "fat-finger safe" than duping key and value pairs.
This just feels like feature creep that might better belong in a
subclass.
And for that matter, why not add an option that works like _AUTO rather
than all the duplicated entries?
Todd Rinaldo
On Jul 15, 2010, at 12:54 PM, Jesse Vincent wrote:
> On Thu, Jul 15, 2010 at 10:37:19AM -0500, Todd Rinaldo wrote:
>>
>> On Jul 8, 2010, at 8:57 AM, Jesse Vincent wrote:
>>
>> Jesse,
>>
>> Onesided-ness is a complementary feature to _AUTO, not a replacement.
>>
>> When you have code that uses _AUTO, Locale fall back happens. The only way to prevent this is to inject a key with an equivalent value to prevent fallback. Sometimes unexpected defaulting might happen. This isn't something you'd use all the time, but in the cases where you do, onesided-ness is more "fat-finger safe" than duping key and value pairs.
>
>
> This just feels like feature creep that might better belong in a subclass.
The only way to subclass it is a monolithic copy/paste. I don't see that it being added hurts anything existing. Even if it is feature creep, what's the down side of that? Honestly I see it as a part of L:M or something else that has to re-implement or duplicate the whole method. That would be a code fork, which I'm trying to avoid here.
> And for that matter, why not add an option that works like _AUTO rather
> than all the duplicated entries?
The goal was to avoid polluting the lexicon namespace with another special variable. Since each lexicon is it's own package, it made more sense to put it outside the lexicon. If that's really what's causing you to hesitate, I have no issues providing an amended patch which uses the lexicon instead to look for the _ONE_SIDED flag.
Todd
Jesse Vincent
>
> The only way to subclass it is a monolithic copy/paste. I don't see that it being added hurts anything existing.
That sounds like something it would be great to fix.
>
> The goal was to avoid polluting the lexicon namespace with another special variable. Since each lexicon is it's own package, it made more sense to put it outside the lexicon. If that's really what's causing you to hesitate, I have no issues providing an amended patch which uses the lexicon instead to look for the _ONE_SIDED flag.
It's certainly _one_ thing that's causing me to hesitate. I'd rather a
special key in the lexicon that works just like the other special key in
the lexicon than something similar that's different. But see above.
Jesse
> Todd
--
Adriano Ferreira
> The only way to subclass it is a monolithic copy/paste. I don't see that it being added hurts anything existing. Even if it is feature creep, what's the down side of that? Honestly I see it as a part of L:M or something else that has to re-implement or duplicate the whole method. That would be a code fork, which I'm trying to avoid here.
The down side is to slow down Locale::Maketext in the common usage
that applications do with it on the wild.
There are HTML sites that perform thousands of L::M lookups per page
and any light change can compromise performance in such applications.
And such compromise comes with no benefits for these applications.
Notice that new conditionals like
if (exists $self->{external_lexicon}) {
}
else {
# previous code
}
are the kind of thing that concerns me.
Adriano Ferreira
>
>
>>
>> The only way to subclass it is a monolithic copy/paste. I don't see that it being added hurts anything existing.
>
> That sounds like something it would be great to fix.
>
I agree whole-heartily with that. By doing the magic necessary to
disentangle the guts of Locale::Maketext from the original
implementation idiosyncrasies (which are for example the use of simple
Perl hashes and replacing its entries as they get interpreted /
compiled), that would be a promising path for the evolution of the
module without inadvertent effects to what already works with
Locale::Maketext.
Of course, that's not an easy task. I already suggested to the
tickets' original requestor that forking Locale::Maketext, approaching
the inability to extend it which is caused by the actual code
structure, and offering such rewrite so it can prove its power / value
(by benchmarks and by being applied to actual code / applications),
that would gives us confidence to think about replacing the original
module.
Sorry for being so conservative, but I really believe that
Locale::Maketext code should not grow to be more complicate (and
expensive in performance and maintenance efforts) that it already is.
Best,
Adriano
Todd Rinaldo
On Jul 23, 2010, at 10:02 AM, Adriano Ferreira wrote:
> On Thu, Jul 22, 2010 at 9:35 PM, Jesse Vincent <je...@fsck.com> wrote:
>>
>>
>>>
>>> The only way to subclass it is a monolithic copy/paste. I don't see that it being added hurts anything existing.
>>
>> That sounds like something it would be great to fix.
>>
>
> I agree whole-heartily with that. By doing the magic necessary to
> disentangle the guts of Locale::Maketext from the original
> implementation idiosyncrasies (which are for example the use of simple
> Perl hashes and replacing its entries as they get interpreted /
> compiled), that would be a promising path for the evolution of the
> module without inadvertent effects to what already works with
> Locale::Maketext.
>
> Of course, that's not an easy task. I already suggested to the
> tickets' original requestor that forking Locale::Maketext, approaching
> the inability to extend it which is caused by the actual code
> structure, and offering such rewrite so it can prove its power / value
> (by benchmarks and by being applied to actual code / applications),
> that would gives us confidence to think about replacing the original
> module.
>
> Sorry for being so conservative, but I really believe that
> Locale::Maketext code should not grow to be more complicate (and
> expensive in performance and maintenance efforts) that it already is.
Thanks everyone for your advice on this one. It's always surprising what'll spark a thread.
I agree with your points. The last thing I want is to slow this critical module down. Look for a patch from me in a month or 2 on re-factoring L:M to make it simpler to subclass.
Thanks,
Todd