[PATCH] D153236: [NFC] Fix potential dereferencing of nullptr.

[Sindhu Chittireddy via Phabricator via cfe-commits]

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