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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/Parse/ParseStmt.cpp:886-887
         TopLevelCase = Case;
-      else
+      else {
+        assert(DeepestParsedCaseStmt && "DeepestParsedCaseStmt cannot be null");
         Actions.ActOnCaseStmtBody(DeepestParsedCaseStmt, Case.get());
----------------
The `assert` doesn't add much value, this seems to be a false positive in the analysis.

If we have no top-level case statement yet, then we get into the `if` branch on line 884, but that then sets `DeepestParsedCaseStmt` on line 890, and that must be non-null because we verified that `Case` is valid above on line 876. So by the time we get into the `else` branch, `DeepestParsedCaseStmt` must be nonnull.

I don't think changes are needed here at all; WDYT?


================
Comment at: clang/lib/Sema/SemaExprObjC.cpp:2441
 
+  assert(receiverTypeInfo && "receiverTypeInfo cannot be null");
   return BuildClassMessage(receiverTypeInfo, ReceiverType,
----------------
schittir wrote:
> 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? 
I think there's a different assertion needed here. The invariant that `BuildClassMessage()` has is that the first argument (the `TypeSourceInfo *` needs to be nonnull if the third argument (the `SourceLocation`) is invalid. So I think this assert should be:

```
assert(((isSuperReceiver && Loc.isValid()) || receiverTypeInfo) &&
       "Either the super receiver location needs to be valid or the receiver needs valid type source information");
```


================
Comment at: clang/lib/Sema/SemaType.cpp:2711-2712
   // Only support _BitInt elements with byte-sized power of 2 NumBits.
   if (CurType->isBitIntType()) {
-    unsigned NumBits = CurType->getAs<BitIntType>()->getNumBits();
+    unsigned NumBits = CurType->castAs<BitIntType>()->getNumBits();
     if (!llvm::isPowerOf2_32(NumBits) || NumBits < 8) {
----------------
Slightly better fix so we don't do "isa" followed by "cast".


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153236/new/

https://reviews.llvm.org/D153236