[llvm-commits] [llvm] r162230 - in /llvm/trunk: lib/CodeGen/MachineVerifier.cpp lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp test/CodeGen/ARM/constants.ll

[Jakob Stoklund Olesen]

On Dec 19, 2012, at 2:05 PM, Chandler Carruth <chandlerc at google.com> wrote:

> On Wed, Dec 19, 2012 at 2:00 PM, Jakob Stoklund Olesen <stoklund at 2pi.dk> wrote:
> 
> On Dec 19, 2012, at 1:40 PM, Chandler Carruth <chandlerc at google.com> wrote:
> 
>> On Wed, Dec 19, 2012 at 1:37 PM, Jakob Stoklund Olesen <stoklund at 2pi.dk> wrote:
>> 
>> On Dec 19, 2012, at 1:10 PM, Chandler Carruth <chandlerc at google.com> wrote:
>> 
>>> On Wed, Dec 19, 2012 at 1:04 PM, Dmitri Gribenko <gribozavr at gmail.com> wrote:
>>> On Wed, Dec 19, 2012 at 11:02 PM, Jakob Stoklund Olesen <stoklund at 2pi.dk> wrote:
>>> >
>>> > On Dec 19, 2012, at 11:01 AM, Dmitri Gribenko <gribozavr at gmail.com> wrote:
>>> >
>>> >> On Tue, Aug 21, 2012 at 12:39 AM, Jakob Stoklund Olesen <stoklund at 2pi.dk> wrote:
>>> >>>       ++MBBI;
>>> >>>       if (MBBI == MF->end()) {
>>> >>>         report("MBB conditionally falls through out of function!", MBB);
>>> >>> -      } if (MBB->succ_size() != 2) {
>>> >>> +      } if (MBB->succ_size() == 1) {
>>> >>
>>> >> Hello Jakob,
>>> >>
>>> >> Sorry to comment on an old commit, but '} if (...' looks suspicious --
>>> >> 'else' is probably missing.  I have just fixed two bugs of this kind
>>> >> in Clang, found by grep '} if'.
>>> >
>>> > Yikes, that does indeed look like a missing else.
>>> >
>>> > Please go ahead and fix it.
>>> 
>>> OK to commit without a testcase?
>>> 
>>> I'd like a testcase. =]
>> 
>> What are you proposing?
>> 
>> Surely the difference in control flow is observable somehow? If this is somehow latent and doesn't actually surface, cool, it just seemed strange to not be able to make this code misbehave visibly.
> 
> If you are concerned about this, you should direct some energy towards the MI serialization project. It would be very welcome.
> 
> I didn't know that testing this was blocked on that, sorry. =/ Wasn't trying to be demanding, it just wasn't clear that there was a particular barrier to adding a test case.
> 
> I'd actually love to spend time here (as would you and others I suspect) but I too have been unable to prioritize it. I actually have some hope that in the next year folks here will end up prioritizing it if others don't get to it sooner. I agree it's an important missing piece infrastructure-wise... 

Sounds good!

Dmitry, please go ahead and commit your fix without a test case.

Thanks,
/jakob

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121219/1b10b77c/attachment.html>