[m-rev.] for review: fix predicate_table_insert bug

[Simon Taylor]

On 24-Oct-2003, Peter Ross <pro at missioncriticalit.com> wrote:
> On Fri, Oct 24, 2003 at 06:14:48PM +1000, Simon Taylor wrote:
> > On 24-Oct-2003, Peter Ross <pro at missioncriticalit.com> wrote:
> > > On Fri, Oct 24, 2003 at 08:58:57AM +1000, Fergus Henderson wrote:
> > > > On 23-Oct-2003, Peter Ross <pro at missioncriticalit.com> wrote:
> > > > > 
> > > > > predicate_table_insert modifies the pred_info before inserting it into
> > > > > the pred_table, this "invalidates" the pred_info predicate_table_insert
> > > > > is called with however there are places in the compiler which use this
> > > > > invalid pred_info for further processing.
> > > > > 
> > > > > Fix this issue by no longer modifying the pred_info inside
> > > > > predicate_table_insert instead record the extra information in a new
> > > > > table.
> > > > 
> > > > That looks fine, thanks!
> > > > 
> > > > [It does lead to a slightly odd/inelegant data structure design,
> > > > where we have two different map(pred_id, ...) maps in the pred_table,
> > > > one mapping to pred_info and one mapping to the accessibility.
> > > > I probably would have been inclined to fix the bug differently,
> > > > by just finding all the places which call predicate_table_insert
> > > > and making sure that they do not rely on the pred_info remaining
> > > > unchanged, and adding some documentation to predicate_table_insert
> > > > to make it clear that it may modify the pred_info before inserting
> > > > it in the table, so the caller should not use the pred_info afterwards.
> > > > But your solution seems more robust.]
> > > > 
> > > I did that first, there is only one place as far as I can tell, however
> > > this is a more robust solution.
> > > 
> > > One improvement could be to modify the pred_table structure itself, so
> > > that it was a tuple (name_accessibility - pred_info).
> > 
> > Why not just return the updated pred_info?  That would be clearer.
> > I'm opposed to this change being committed.
> > 
> The question for me is, should the information about the accessibility
> of the predicate be in the pred_info?
>
> It is only needed for predicate_table_restrict.

Currently, but there's no reason it couldn't be used elsewhere.

> So for me, it makes sense for that information to be local to the
> predicate_table data structure and therefore not to be recorded in
> the pred_info.

The current compiler design places all information about a predicate
in one place, the pred_info.  I don't see any compelling reason here
to change that.

Simon.
--------------------------------------------------------------------------
mercury-reviews mailing list
post:  mercury-reviews at cs.mu.oz.au
administrative address: owner-mercury-reviews at cs.mu.oz.au
unsubscribe: Address: mercury-reviews-request at cs.mu.oz.au Message: unsubscribe
subscribe:   Address: mercury-reviews-request at cs.mu.oz.au Message: subscribe
--------------------------------------------------------------------------