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

[Dmitri Gribenko]

On Thu, Dec 20, 2012 at 12:19 AM, Eli Bendersky <eliben at google.com> wrote:
> On Wed, Dec 19, 2012 at 2:16 PM, Dmitri Gribenko <gribozavr at gmail.com> wrote:
>> On Wed, Dec 19, 2012 at 11: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. =]
>>>
>>> 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.

Seems like only Google Code Search can do that, other tools just drop
the brace...

http://code.google.com/codesearch#search/&q=^\s*}\s*if%20lang:^c$&type=cs
http://code.google.com/codesearch#search/&q=^\s*}\s*if%20lang:^c%2B%2B$&type=cs

All these look like real issues (but most of them happen to test for
conditions that are exclusive with conditions already handled, so they
don't affect correctness).

But this might warn for weird code formatting styles (or lack of one...)

if (x) {a = x;} if (y) {a = y;}

Dmitri

-- 
main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
(j){printf("%d\n",i);}}} /*Dmitri Gribenko <gribozavr at gmail.com>*/