schittir added inline comments.
================
Comment at: clang/lib/Parse/ParseStmt.cpp:887
+ else {
+ assert(DeepestParsedCaseStmt && "DeepestParsedCaseStmt cannot be
null");
Actions.ActOnCaseStmtBody(DeepestParsedCaseStmt, Case.get());
----------------
Fznamznon wrote:
> The assert that looks like `assert(x && "x should not be null")` seems
> strange. Failed `assert(x)` implies that `x` should not be null. If there is
> a message, a message saying what is wrong and why is much more useful.
Would you suggest removing the message and changing it to
```
assert(DeepestParsedCaseStmt);
```
I am not sure what message would be useful here.
================
Comment at: clang/lib/Sema/SemaExprObjC.cpp:2441
+ assert(receiverTypeInfo && "receiverTypeInfo cannot be null");
return BuildClassMessage(receiverTypeInfo, ReceiverType,
----------------
Fznamznon wrote:
> That seems to be a strange place before and after changes. With or without
> change, when `ReceiverType.isNull()` is true we just end up passing `nullptr`
> as `receiverTypeInfo ` to the `BuildClassMessage` which doesn't seem to be
> checking its non-nullness before dereferencing it, even though its
> description says that `receiverTypeInfo` can be null.
> I guess it is fine to pass `nullptr` to a function whose description says so,
> but the non-nullness check inside of it should be probably a bit more obvious
> than it is right now.
I see your point about passing `ReceiverTypeInfo` as nullptr to
`BuildClassMessage` method - it seems ok to do that.
Would it make sense to add an `assert(ReceiverTypeInfo);` inside the method as
way of making the non-nullness check more obvious?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D153236/new/
https://reviews.llvm.org/D153236
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
