[PATCH] D139148: Fix nullptr dereference found by Coverity static analysis tool

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added a comment.

> I am bit afraid about release builds:
>
> if (!Initializer)
>
>   return;
>
> Is this semantically incorrect?

I think so. And now I think I see a path where the proposed `assert` is incorrect as well. The `else` branches at lines 5919, 5921, and 5923 appear to handle cases where `Initializer` may be null.

  clang/lib/Sema/SemaInit.cpp:
   5824   // Handle default initialization.
   5825   if (Kind.getKind() == InitializationKind::IK_Default) {
   5826     TryDefaultInitialization(S, Entity, Kind, *this);
   5827     return;
   5828   }
          <Proposed assertion>
   ....
   5835   if (const ArrayType *DestAT = Context.getAsArrayType(DestType)) {
   5836     if (Initializer && isa<VariableArrayType>(DestAT)) {
   ....
   5912     else if (S.getLangOpts().CPlusPlus &&
   5913              Entity.getKind() == InitializedEntity::EK_Member &&
   5914              Initializer && isa<InitListExpr>(Initializer)) {
   ....
   5918     } else if (DestAT->getElementType()->isCharType())
   5919       SetFailed(FK_ArrayNeedsInitListOrStringLiteral);
   5920     else if (IsWideCharCompatible(DestAT->getElementType(), Context))
   5921       SetFailed(FK_ArrayNeedsInitListOrWideStringLiteral);
   5922     else
   5923       SetFailed(FK_ArrayNeedsInitList);
   5924
   5925     return;
   5926   }

Perhaps the` assert` should be added after line 5926 above. I would be concerned about adding a return statement conditional on `Initializer` being null there though. If the intention is that an initializer must be present after that point, an early return could result in a miscompile; I'd rather have a crash.

I think the checks for `Initializer` being non-null following the addition of an `assert` should be removed.


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

https://reviews.llvm.org/D139148