[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
clattner at apple.com
Tue Nov 4 02:35:39 PST 2008
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
>>> 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. :)
More information about the cfe-commits