[cfe-commits] r76821 - in /cfe/trunk: include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/Sema.h lib/Sema/SemaDecl.cpp test/Sema/return.c test/SemaCXX/return.cpp test/SemaObjC/return.m

[Mike Stump]

On Jul 23, 2009, at 1:11 AM, Daniel Dunbar wrote:
> Nit: this is just BFS, not mark and sweep?

Fixed.

> I switch this to a BitVector.

Thanks.

> The end iterator doesn't need to be recalculated, nor does the
> variable declaration need to be outside the loop, please fix to be
> more consistent with other code.

Fixed.

> No space after ! please. Three '(*i)' expressions is pushing my
> limits, just to save a line of code. I can live with this, but please
> spill '(*i)' in next loop into a temporary, especially to get rid of
> **i.

Fixed.

> Again, doesn't need to be outside loop.

Fixed.

> We should also check for noreturn message sends, I believe.

The CFG code doesn't do noreturn in that case yet.  I pushed a FIXME  
in for now.

> The indentation and bracing here is inconsistent with clang

Fixed.

> I have a tingling suspicion the logic is also overly complicated,

I know what you mean, but I plan on using the other bits for follow on  
work.  :-)  When the compiler is done, I think I'll use all the bits I  
calculate.

> Please reduce nesting

Fixed.

> Please use || with spaces.

Fixed.

>> +    // This says never for calls to functions that are not marked  
>> noreturn, that
>> +    // don't return.  For people that don't like this, we  
>> encourage marking the
>> +    // functions as noreturn.
>
> This doesn't make sense. That is, I understand what it means, but the
> prose is trying hard to prevent that from occurring. ;)

I left in wording for previously used return values, which made it  
unreadable.  I've fixed it.

> Typo ('cascding').

Fixed.

> Why is this needed?

I've added a comment in the code to explain further, since I would  
like to see this fixed better.

> It would be nice to get the 'noreturn function does return warning'
> too, which is now trivial to add.

Yup.

> Did you do any timing to see what the impact of CFG construction on  
> Sema was?

Nope.  I have a version of the code that is much faster, if slightly  
less complete, if we wanted to radically speed it up.