Trying to help elegant_merge live up to its name.

Should be good to go.  Witnesses and tests passed locally.

Thanks,
Jason

Sat Nov 14 14:31:05 PST 2009  Jason Dagit <dagit@codersbase.com>
  * clean up elegant_merge

Sat Nov 14 14:42:43 PST 2009  Jason Dagit <dagit@codersbase.com>
  * replace commutex with commute in elegant_merge

Attachments

• clean-up-elegant_merge.dpatch
• unnamed

On Sat, 14 Nov 2009, Jason Dagit wrote:

> Sat Nov 14 14:31:05 PST 2009  Jason Dagit <dagit@codersbase.com>
>  * clean up elegant_merge

> -elegant_merge (p1 :\/: p2) =
> -  case commutex (p1 :< invert p2) of
> -  Just (ip2':<p1') -> case commutex (p1' :< p2) of
> -                      Nothing -> Nothing -- should be a redundant check
> -                      Just (_:<p1o) -> if unsafeCompare p1o p1
> -                                       then Just (invert ip2' :/\: p1')
> -                                       else Nothing
> -  Nothing -> Nothing
> +elegant_merge (p1 :\/: p2) = do
> +  ip2' :< p1' <- commutex (p1 :< invert p2)
> +  _ :< p1o <- commutex (p1' :< p2)
> +  if unsafeCompare p1o p1 -- should be a redundant check
> +    then return $ invert ip2' :/\: p1'
> +    else Nothing

You've moved the "should be a redundant check" comment from the second 
commutex call to the unsafeCompare. I guess that the comment really 
should cover both of these things, do you agree?

BTW the code could be further elegantised by using "guard".

If you agree about the comment, I suggest I accept this patch as it 
stands, particularly since other stuff depends on it, and one of us sends 
a follow-on to clarify the comment.

> Sat Nov 14 14:42:43 PST 2009  Jason Dagit <dagit@codersbase.com>
>  * replace commutex with commute in elegant_merge

Looks fine.

Ganesh

On Wed, Nov 18, 2009 at 6:26 AM, Ganesh Sittampalam <ganesh@earth.li> wrote:

> On Sat, 14 Nov 2009, Jason Dagit wrote:
>
>  Sat Nov 14 14:31:05 PST 2009  Jason Dagit <dagit@codersbase.com>
>>  * clean up elegant_merge
>>
>
>  -elegant_merge (p1 :\/: p2) =
>> -  case commutex (p1 :< invert p2) of
>> -  Just (ip2':<p1') -> case commutex (p1' :< p2) of
>> -                      Nothing -> Nothing -- should be a redundant check
>> -                      Just (_:<p1o) -> if unsafeCompare p1o p1
>> -                                       then Just (invert ip2' :/\: p1')
>> -                                       else Nothing
>> -  Nothing -> Nothing
>> +elegant_merge (p1 :\/: p2) = do
>> +  ip2' :< p1' <- commutex (p1 :< invert p2)
>> +  _ :< p1o <- commutex (p1' :< p2)
>> +  if unsafeCompare p1o p1 -- should be a redundant check
>> +    then return $ invert ip2' :/\: p1'
>> +    else Nothing
>>
>
> You've moved the "should be a redundant check" comment from the second
> commutex call to the unsafeCompare. I guess that the comment really should
> cover both of these things, do you agree?
>

When I read the code, what looked redundant to me was the check that p1o and
p1 are the same.  On the other hand, the comment seems odd and I was very
tempted to remove it.  It could refer to propagating the Nothing value as
well.  Is it possible we should remove the second commutex and the unsafe
compare because they are redundant checks (paranoid checks?)?


>
> BTW the code could be further elegantised by using "guard".
>

Oh, I hadn't thought of that.  I'll try to figure out how that should work
in a follow up patch if we decide to keep the unsafeCompare...

>
> If you agree about the comment, I suggest I accept this patch as it stands,
> particularly since other stuff depends on it, and one of us sends a
> follow-on to clarify the comment.


I agree we should do something as a follow up.  Would it be okay if we
removed the second commutex and the unsafeCompare?

Jason

Attachments

• unnamed

On Wed, 18 Nov 2009, Jason Dagit wrote:

> I agree we should do something as a follow up.  Would it be okay if we
> removed the second commutex and the unsafeCompare?

I think that's reasonable.

Ganesh

This was applied a while ago but darcswatch didn't notice