Re: FYI: ConditionalSet.pm

[Raja R Harinath]

Hi,

Alexandre Duret-Lutz <address@hidden> writes:
[snip]
> --- automake.in       13 Nov 2002 21:58:26 -0000      1.1385
> +++ automake.in       14 Nov 2002 16:01:55 -0000
[snip]
> @@ -6550,7 +6533,7 @@
>       if (exists $targets{$var}
>           && (! defined $cond || exists $targets{$var}{$cond}))
>         {
> -         for my $tcond ($cond || target_conditions ($var))
> +         for my $tcond ($cond || ! target_conditions ($var)->false)
>             {
>               prog_error ("\$targets{$var}{$tcond} exists but "
>                           . "\$target_owner doesn't")

This appears to be bogus.  I think it should be

  for my $tcond ($cond || target_conditions ($var)->conds)

> --- /dev/null 1 Jan 1970 00:00:00 -0000
> +++ lib/Automake/ConditionalSet.pm    14 Nov 2002 16:01:58 -0000
[snip]
> +sub new ($;@)
[snip]
> +      # If we see true, then the whole set is true!
> +      if ($cond->true && $#conds > 0)
> +     {
> +       return new Automake::ConditionalSet $cond;
> +     }

This is "normalizing" the ConditionalSet in this special case.  Won't
this cause loss of information?  Say:

  foo = 1
  if SOMECOND
    foo = 2
  endif

Then variable_conditions('foo') should be ("TRUE", "SOMECOND_TRUE"),
not ("TRUE"), I think.

[snip]
> +sub true ($ )
> +{
> +  my ($self) = @_;
> +  # To know whether a ConditionalSet covers all
> +  # we invert it.  invert() will set $self->{'true'}.
> +  $self->invert unless exists $self->{'true'};
> +  return $self->{'true'};
> +}

Isn't this simply

  $self->invert->false

Since $self->invert is cached, I don't see why you need to cache $self->true.

> +sub _permutations_worker (@)
> +{
> +  my @conds = @_;
> +  return () unless @conds;
> +
> +  my $cond = shift @conds;
> +  (my $neg = $cond) =~ s/TRUE$/FALSE/;
> +
> +  # Recurse.
> +
> +  # Don't merge `FALSE' conditions, since this will just create
> +  # a false Conditional, and we'll drop them later in ConditionalSet.
> +  # (Dropping them now limits the combinatoric explosion.)
> +  my @ret = ();
> +  foreach my $c (&_permutations_worker (@conds))
> +    {
> +      push (@ret, $c->merge_conds ($cond));
> +      push (@ret, $c->merge_conds ($neg)) if $neg ne 'FALSE';
                                             ^^^^^^^^^^^^^^^^^^
If you don't let $cond be 'TRUE', this shouldn't be necessary.
I noticed that the original code comment said something like

  permutations("COND1_TRUE", "TRUE") = ("COND1_TRUE TRUE", "COND1_FALSE TRUE")

which is what your code would return if you didn't normalize the
ConditionSet earlier :-).  I think

  permutations("COND1_TRUE", "TRUE") = ("COND1_TRUE", "COND1_FALSE")

should be sufficient.

> +Calling $<$set-E<gt>permutations> will return the following Conditional set.
> +
> +  new Automake::ConditionalSet
> +    (new Automake::Conditional ("COND1_TRUE", "COND2_TRUE", "COND2_TRUE"),
> +     new Automake::Conditional ("COND1_FALSE","COND2_TRUE", "COND2_TRUE"),
> +     new Automake::Conditional ("COND1_TRUE", "COND2_FALSE","COND2_TRUE"),
> +     new Automake::Conditional ("COND1_FALSE","COND2_FALSE","COND2_TRUE"),
> +     new Automake::Conditional ("COND1_TRUE", "COND2_TRUE", "COND2_FALSE"),
> +     new Automake::Conditional ("COND1_FALSE","COND2_TRUE", "COND2_FALSE"),
> +     new Automake::Conditional ("COND1_TRUE", "COND2_FALSE","COND2_FALSE"),
> +     new Automake::Conditional ("COND1_FALSE","COND2_FALSE","COND2_FALSE"));
                                                                ^^^^^^^^^^^^
ITYM "COND3_..." there.

- Hari
-- 
Raja R Harinath ------------------------------ address@hidden