Re: [Monotone-devel] automate show_conflict

[Stephen Leake]

"Zack Weinberg" <address@hidden> writes:

> No comment on anything else - I will try to find time to read the
> patch in the next few days - but
>
> On Thu, Apr 17, 2008 at 6:22 PM, Thomas Keller <address@hidden> wrote:
>>  +  boost::shared_ptr<roster_t const> 
>> left_roster(db_adaptor.rosters[db_adaptor.left_rid]);
>>  +  I(0 != left_roster);
>>  +  boost::shared_ptr<roster_t const> 
>> right_roster(db_adaptor.rosters[db_adaptor.right_rid]);
>>  +  I(0 != right_roster);
>>
>>  I don't know much of boost::shared_ptr, but basically any of both
>> invariants would fire if the db_adaptor.rosters map does not have the
>> revision roster cached, correct? If we need invariants there, wouldn't it be
>> better to check the root of the paranoia, i.e.
>>
>>  I(db_adaptor.rosters.find(db_adaptor.left_rid) != db_adaptor.rosters.end())
>
> ... please don't do things that will make it do tree searches twice in
> a row in the common case.  I'd probably write this as
>
> typedef map<revision_id, shared_ptr<roster_t const> >::iterator
> rid_roster_iterator;
> rid_roster_iterator left_roster_p = db_adaptor.rosters.find(left_rid);
> I(left_roster_p != db_adaptor.rosters.end());
> // ... etc ...
>
> but even that looks rather ugly :-(

My prefered solution is to use Ada instead of C++. Then the compiler
inserts this check implicitly for me, and optimizes it away whenever
it can :).

The current I(0 != ..) looks cleanest/clearest to me.

-- 
-- Stephe