<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 18 November 2013 18:19, Paul Bone <span dir="ltr"><<a href="mailto:paul@bone.id.au" target="_blank">paul@bone.id.au</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class="im"><span style="color:rgb(34,34,34)">I took a look on github, your commit message doesn't describe your changes.</span><br></div>
It is generally the same as the message used on the mailing lists but<br>
doesn't require the detail of the above notes.<br></blockquote><div><br></div><div>I haven't yet made the pull request. This is a pre-PR glimpse. When I make the formal pull request it will be exhaustively documented, trust me. ;)</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class="im"><br>
<br>> +:- pred pqueue.is_empty(pqueue(K,V)::in) is semidet.<br><br>
</div>yeah, the bikeshed should have a space after the comma and before the "V".<br>
Just to make it conform to the existing style.<br></blockquote><div><br></div><div>Fair cop. That was a typo I didn't catch in my final review of the code. Fixed.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class="im"><span style="color:rgb(34,34,34)">How long are lines in your text editor? We like to limit lines to 76</span><br></div>
columns so taht a diff plus an e-mail reply doesn't cause wrapping.<br></blockquote><div><br></div><div>Ah. I have my right column marked at 80. I'll adjust to 76 for Mercury code. Fixed for my additions, but I've noted more than a few lines in</div>
<div>old code that exceed this. Shall I fix them as well?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Why isn't the K returned in the predicate version of peek. I'd imagine in<br>
some cases it'd be useful. If a caller doesn't want it then they can pass<br>
the '_' variable.<br></blockquote><div><br></div><div>Hmmm... You want different behaviour in the predicate and the function? It's doable, but to me a bit weird. Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Comments on declarations should always include an extra line with a %<br>
character.<br></blockquote><div><br></div><div>Another typo, result of a last-second editing change. Fixed.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class="im"><br>> + error("pqueue.det_peek/2: empty priority queue")<br><br>
</div>So you can write:<br>
<br>
unexpected($module, $pred, "empty priority queue")<br></blockquote><div><br></div><div>Fixed.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class="im"><br>
> +%---------------------------------------------------------------------------%<br>
> +<br>
> pqueue.insert(!.PQ, K, V) = !:PQ :-<br>
> pqueue.insert(K, V, !PQ).<br>
><br>
<br>
</div>The style specifies one whitespace line at most between anything.<br></blockquote><div><br></div><div>I'm not sure I understand. That's old code. And I only see one line of whitespace anyway.</div><div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class="im"><br>
> @@ -150,7 +189,7 @@<br>
> V = V0<br>
> else<br>
> error("pqueue.det_remove/4: empty priority queue")<br>
> - ).<br>
> + ).<br>
><br>
<br>
</div>Does this add or remove whitespace. It's not written in the style guide but<br>
at least Zoltan and I take measures to avoid trailing whitespace.<br></blockquote><div><br></div><div>It removes a trailing whitespace that was in the old code. My editor does that for me automatically unless I explicitly turn off the behaviour.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class="im">> +pqueue.merge(PQ1, PQ2) = PQ3 :-<br>
> + pqueue.merge(PQ1, PQ2, PQ3).<br>
> +<br>
> +pqueue.merge(empty, PQ2, PQ2).<br>
> +pqueue.merge(PQ1@pqueue(_,_,_,_,_), empty, PQ1).<br>
> +pqueue.merge(PQ1@pqueue(_,_,_,_,_), pqueue(_,K,V,L,R), PQ3) :-<br>
> + pqueue.merge(PQ1, L, IPQ1),<br>
> + pqueue.merge(IPQ1, R, IPQ2),<br>
> + pqueue.insert(K, V, IPQ2, PQ3).<br>
<br>
</div>I suggest for the arguments using letters, A and B. and numbers for the<br>
version of something.<br></blockquote><div><br></div><div>OK, will do.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Note that your merge is faster if the queue in the second argument is<br>
smaller. This is not bad, you have to pick one. You should note this in<br>
the comment on the declaration for merge.<br></blockquote><div><br></div><div>I think this eliminates the need for the caller to worry about it?:</div><div><br></div><div><div><font face="courier new, monospace">pqueue.merge(A, B, C) :-</font></div>
<div><font face="courier new, monospace"> ( pqueue.length(A) =< pqueue.length(B) -></font></div><div><font face="courier new, monospace"> pqueue.merge2(A, B, C)</font></div><div><font face="courier new, monospace"> ;</font></div>
<div><font face="courier new, monospace"> pqueue.merge2(B, A, C) ).</font></div><div><font face="courier new, monospace"><br></font></div><div><font face="courier new, monospace">:- pred pqueue.merge2(pqueue(K, V)::in, pqueue(K, V)::in, pqueue(K, V)::out)</font></div>
<div><font face="courier new, monospace"> is det.</font></div><div><font face="courier new, monospace"><br></font></div><div><font face="courier new, monospace">pqueue.merge2(empty, B, B).</font></div>
<div><font face="courier new, monospace">pqueue.merge2(A@pqueue(_, K, V, L, R), empty, A).</font></div><div><font face="courier new, monospace">pqueue.merge2(pqueue(_, K, V, L, R), !.PQ@pqueue(_, _, _, _, _), !:PQ) :-</font></div>
<div><font face="courier new, monospace"> pqueue.merge2(L, !PQ),</font></div><div><font face="courier new, monospace"> pqueue.merge2(R, !PQ),</font></div><div><font face="courier new, monospace"> pqueue.insert(K, V, !PQ).</font></div>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="im">> P.S. The bike shed should be cerulean. ;-)<br>
<br>
</div>Red goes faster.<br></blockquote><div><br></div><div>Going faster to the wrong place is not what I'm aiming for. Correctness before speed. </div></div><div><br></div>-- <br>"Perhaps people don't believe this, but throughout all of the discussions of entering China our focus has really been what's best for the Chinese people. It's not been about our revenue or profit or whatnot."<br>
--Sergey Brin, demonstrating the emptiness of the "don't be evil" mantra.
</div></div>