Re: Need syntax/small footprint help



On Sun, 27 Mar 2005 10:28:12 +0000, Brian McCauley <nobull@xxxxxxxx>
wrote:

>
>
>robic0@xxxxxxxxx wrote:
>
>> Hello, last month I wrote a rather large 10,000 line program that
>> parses xml (expat). The data is read into compound structures
>> then analyzed later. Below is one of the subroutines. I would like
>> to make it smaller so that its easier to read and understand and
>> also make the regular expressions less complicated for performance.
>> Any ideas?
>>
>> ---------------------------
>> sub eCHK_RegDel ## -- checks Redundant Regkey Delete // Regkey
>> exists --
>> {
>> my ($details) = @_;
>> return 1 if (@{$details} == 0);
>
> return 1 unless @$details;
yes, the same
>
>But actually this is, I think, redundant. There is no point since if
>@$details is empty so will @regdel so nothing will happen anyhow.
>
return from @details empty check comes b4 @regdel population?

>Why are you returning 1? You don't seem to be bothering about the
>returning anything in other situations so I infer that nothing is
>considering the return value of eCHK_RegDel.

right, I paid no attention to the return condition at the end of the
sub. where its called from pays no attention to return codes as of
yet.
>
>> my @regdel = ();
>> for (@$details) {
>> my $aref = $_->[0];
>> push (@regdel, $aref) if ($aref->[5] eq $REGISTRY && $aref->[6] eq
>> $DELETE);
>> }
>
>my @regdel =
> grep { $_->[5] eq $REGISTRY && $_->[6] eq $DELETE )
> map { $_->[0] } @$details;
>
>
>> if (@regdel)
>> {
>
>Again this is redundant. "If the next line would do nothing then don't
>do it". But if the next line would do nothing there's no cost in doing
>it anyhow.

It may look weird but I assure you its the same push return addr onto
the stack register without a goto.
also, its proper form that more code may end up below "if" statement,
large code dictates you leave the door open.
>
>> ## check that KeyName exists
>> for (@regdel) {
>> my $aref = $_;
>
>You don't need the value in $_ so simply:
>
> for my $aref (@regdel) {
>
writing freehand code from top to bottom, it is a rarety you will know
what the final outcome will be. not alot of time to go back...
but sometimes
>> next if (exists ($aref->[4]{$KEYNAME}) &&
>> length($aref->[4]{$KEYNAME}) > 0);
>
>You mean defined() not exists().
>
>length() can never be negative so >0 is redundant.
>
Not true... $aref->[4]{$KEYNAME} is dynamic. it MUST exist
and if it does is sometimes zero length, which is a condition
that is unacceptable...

>There could be a case for simply using no warnings 'uninitialized' here.
>
>> # log error
>> ## 0701 -- test 07,01
>> ReportItem ('E', $CLOGINFO, $CERRLOG, '0701', glicn($aref), (0,0),
>> '', $XML_File, 1,0,1);
>> }
>> ## check redundant regkey deletes
>> my $first_kname = '';
>> my $first_vname_exists = 0;
>> my $aref_first = undef;
>>
>> my @regx_esc_codes = ( "\\", '/', '(', ')', '[', ']', '?', '|',
>> '+', '.', '*', '$', '^', '{',
>> '}', '@' );
>
>This is invarient and should be taken outside the loop or maybe even
>outside the subroutine. But you don't need it at all probably.

Not true... it is not inside a loop. global scope had no use for
it.... in general, scoping in Perl is just like C++, dump it as soon
as possible !!

>
>> for (@regdel)
>> {
>> my $aref = $_;
>> next if (!(exists ($aref->[4]{$KEYNAME}) &&
>> length($aref->[4]{$KEYNAME}) > 0));
>
>Why the second loop? Why not a do it in a single pass?
>
for alot of reasons... one is the side affects of trying to do such
a complicated maneuver all at once... generally a good
precautionarry measure especially since the first does not
slow down the second at all, since the @regdel array of
structures has been pre-conditioned via a sort engine
that maximizes this kind of check ahead of time.
Pre-fetching like this occurs sometimes 6 levels deep
before analysis begins. Its a trade-off where clarity wins
vs. bug introduction, but the engines must be designed
for this type of thing ahead of time... which were.

>> my $kname = $aref->[4]{$KEYNAME};
>> my $vname = '';
>> my $kv = 'key (branch)';
>> if (exists ($aref->[4]{$VALUENAME})) {
>> if (length($aref->[4]{$VALUENAME}) > 0) {
>> $vname = $aref->[4]{$VALUENAME};
>> # strip leading spaces
>> while ($vname =~ s/^ //) {}
>
> s/^ +//;
>
>> }
>> $kv = 'value';
>> }
>> for (@regx_esc_codes)
>> {
>> my $tc = $_;
>> my $xx = "\$kname =~ s/\\$tc/\\\\\\$tc/g;"; # code template for
>> regex
>> eval $xx;
>> #print "$xx\n";
>> }
>
>Do not use eval unless there is a reason to do so. The above is more simply
>
#--#
>for my $tc (@regx_esc_codes) {
> $kname =~ s/(\Q$tc\E)/\\$1/g;
>}
#--#
I don't know what this is... I guess I should check it.
If you can resolve ALL the "$tc" with the "\Q"
in regx then this should work... have you tried this method yourself?
Without eval on the @regx codes you can't esc an "odd" number
of '\' without muting the '$' in "$tc". I hope it works I will try it
tommorow. @regx is array of single chars, this won't work on
expressions as are the eval in the dynamic strings compared later.
Course thats why its done here right....
>
>But I suspect you simply wanted
>
> $kname = quotemeta $kname;
>
>But I'm guessing you wouldn't need to do anything if you got rid of some
>more of the pointless eval()s in your code.
>
>All in all I think you are working way too. Hard. I'll stop now with
>the line-by-line and go have another look at your code in its entirity.
> Get back to you soon.

You know, I'm being paid far too much money for it too .... hehe!!!

.