[cfe-commits] r58509 - in /cfe/trunk: include/clang/Basic/DiagnosticKinds.def lib/AST/Expr.cpp lib/Sema/Sema.h lib/Sema/SemaExprCXX.cpp lib/Sema/SemaInherit.cpp lib/Sema/SemaInherit.h lib/Sema/SemaOverload.cpp test/SemaCXX/condition.cpp test/SemaCXX/dynamic-cast.cpp test/SemaCXX/static-cast.cpp

[Chris Lattner]

On Nov 3, 2008, at 7:28 AM, Sebastian Redl wrote:

> Chris Lattner wrote:
>> On Oct 31, 2008, at 7:43 AM, Sebastian Redl wrote:
>>> URL: http://llvm.org/viewvc/llvm-project?rev=58509&view=rev
>>> Log:
>>> Implement semantic checking of static_cast and dynamic_cast.
>>
>> Very nice Sebastian, thank you for your contribution!
>>
>> Some thoughts below.  I'm leaving the "deep" issues to Doug, but  
>> I'll nit pick some details :)
> Thanks for the review. I'll address your comments below and update  
> the code as soon as I get my next piece of work out of the way.

Thanks! No problem,


>> This is another very personal issue, but I'd generally prefer to  
>> merge these into the same if, and use "||".  There should also be a  
>> small comment explaining why you reject non record types.
> I'll merge them. As for the comment, I think it's unnecessary. The  
> function checks whether this is a cast in a class hierarchy - it  
> should be completely obvious why these need to be records.

While it is obvious to you, it may not be obvious to all readers.  The  
comment doesn't need to be long, just exactly what you said there  
would be enough.  Please have mercy on those of us that are not C++  
language lawyers (yet!), or that do not take time to understand the  
full context for the code. :)

-Chris