[m-rev.] for review: fix mantis bug 544

Zoltan Somogyi zoltan.somogyi at runbox.com
Mon Feb 7 17:34:09 AEDT 2022


2022-02-07 17:19 GMT+11:00 "Julien Fischer" <jfischer at opturion.com>:
> Once this is committed, 

It is committed, but you may want to wait for ...
 
> On Mon, 7 Feb 2022, Zoltan Somogyi wrote:
> 
>> For review by Julien.
>>
>> As mentioned in a comment, we could eliminate one source
>> of overhead added by this diff by adding a common_subset
>> predicate to assoc_list.m. Would anyone object to this?
> 
> No objections; ideally map.m and assoc_list.map should support the same
> (or at least a very similar) set of operations, both being map ADTs.

... this, which I am starting on now.

> That's one reason the LLDS code generator is not affected; the other is
> simply that it introduces trail operations directly and does not use
> add_trail_ops.m.

I added a mention.

>> +    % closure to the values associated with the key in MapA and MapB.
>>      % Fail if and only if this closure fails on the values associated
>>      % with some common key.
> 
> 
> More generally, I would note that there does not seem to be anything in the test
> suite that exercises common_subset.

Unfortunately, there are many more stdlib preds in that same boat.

>> --- a/trace/mercury_trace_cmd_browsing.c
>> +++ b/trace/mercury_trace_cmd_browsing.c
>> @@ -847,6 +847,7 @@ MR_trace_cmd_dump(char **words, int word_count, MR_TraceCmdInfo *cmd,
>>              const char  *name;
>>
>>              MR_convert_arg_to_var_spec(words[1], &var_spec);
>> +            // MR_trace_parse_var_path
>>              problem = MR_lookup_unambiguous_var_spec(var_spec,
>>                  &type_info, &value, &name);
>>              if (problem == NULL) {
> 
> Did you mean to add this?

No. I followed all your other suggestions.

> The rest looks fine -- thanks for looking into that.

You are welcome.

Zoltan.


More information about the reviews mailing list