From: Larry D. <ldo...@re...> - 2008-01-28 21:58:48
|
Cary - On Mon, Jan 28, 2008 at 01:31:22PM -0800, Cary R. wrote: > Though I would remove the disabled "if" since it serves no purpose other > than to confuse someone later. OK. I almost took it out, but left it in to keep the diff small. Also, I wasn't yet sure that postponing the test would actually work. Uwe, have you tested the result? Has anyone run this patch through the test suite? > I would also change the comment to be "If > the entire value is shifted away connect the output to a constant V0 or > for arithmetic right shift ">>>" connect all bits to the MSB." OK. Except that additional detail applies whether or not all of the value is shifted away. How about putting it in front of the if (op_ == 'R') { block? > What is interesting is that if the conditional was correct >= vs > we > would have known exactly where the bug or in this case missing > functionality was and no incorrect code would have been generated. Right. I noticed that part a couple of days ago, and almost submitted a patch to just fix that conditional. But that seemed a little non- constructive, and I also wanted to understand how the problem propagated. I'm quite curious to find out if that new assert() in netlist.cc ever gets triggered in the wild. - Larry |