Re: [Monotone-devel] comments on merging nvm.error-handling?

[Timothy Brownawell]

On Tue, 2009-01-20 at 10:01 +0100, Markus Wanner wrote:
> Hi,
> 
> I also had this on my list for reviewing, but didn't get around doing
> much...

I suppose I should remember to wait longer next time...

> Please remove commented out code, as in transforms.hh, ~ line 98. And
> sanity.hh, ~ line 21. Maybe other places as well.

Done, thanks for the reminder.

> I for one am always confused about the conditions in E(), because I
> always have to look up which direction they work. And it gets especially
> confusing with negated conditions. The additional origin argument
> doesn't increase readability either.
> 
> I'd even prefer writing out the condition to de-obfuscate, for example not:
> 
>    E(!null_id(edge_old_revision(i)), made_from,
>      F("Merge revision has a null parent"));
> 
> but better:
> 
>    if (null_id(edge_old_revision(i))
>      E(made_from, F("Merge revision has a null parent"));
> 
> Or similar. Those very few additional characters would IMO help
> readability and understandability a lot. But maybe that's just me.

It's like I() or the canonical "assert()", except with a message (and
now a caused-by). This might be good to keep the same, just to be
consistent.

It also logs the text of the condition if the debug flag is set so you
maybe don't have to keep looking up file:line to see what broke, hard to
do if the condition isn't an argument. But you'll presumably be digging
through the source *anyway* in that case, so...

-- 
Timothy

Free (experimental) public monotone hosting: http://mtn-host.prjek.net