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

[Eli Bendersky]

>>>>>
>>>>> 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. =]
>>>>
>>>> Dmitri, want to try your hand at building a Clang warning for this? I just
>>>> chatted with Richard and he seemed to think it could work by warning in the
>>>> Parser when looking for an 'else' token, finding an 'if' token, and the '}'
>>>> and 'if' tokens' spelling locations are on the same line. (or checking the
>>>> start-of-line flag on the 'if' token, but Richard thought there might be
>>>> problems there).
>>>
>>> That's a good idea.  So far LLVM/Clang had three cases of this, and
>>> all are true bugs.
>>>
>>
>> I wonder if other open-source projects have this bug somewhere as
>> well. Maybe some code-search online thingy can help find out. If you
>> go ahead and add a new Clang warning, it's important to know whether
>> there are going to be legit uses that will be warned about.
>
> Yes, there are several popular open-source projects with this bug. My
> survey of the hits for "} if" found a double-digit number of bugs,
> some cases where adding the 'else' would make no semantic difference
> (the previous block ended in return, continue, break, exit, etc), and
> only one case where the intent was clearly to have two separate if
> statements (and even in that case, the newline appears to have been
> accidentally forgotten).

Sounds like a good candidate for a Clang warning, then (depending on
how "linty" you want Clang to be, of course).

Eli