tahonermann requested changes to this revision.
tahonermann added a comment.
This revision now requires changes to proceed.
Per added comments, I think we should look for a guarantee that `Initializer`
is non-null earlier in the function. If there is, then we could remove a bunch
of the current existence checks rather than adding more.
================
Comment at: clang/lib/Sema/SemaInit.cpp:5824-5828
// Handle default initialization.
if (Kind.getKind() == InitializationKind::IK_Default) {
TryDefaultInitialization(S, Entity, Kind, *this);
return;
}
----------------
This block handles default initialization and unconditionally performs a
return. I wonder if this effectively guarantees that `Initializer` is non-null
if this block is not entered.
================
Comment at: clang/lib/Sema/SemaInit.cpp:5933
if (TryOCLSamplerInitialization(S, *this, DestType, Initializer))
return;
----------------
The use of `initializer` here looks questionable too;
`TryOCLSamplerInitialization()` will dereference it without a check if both
`S.getLangOpts().OpenCL` and `DestType->isSamplerT()` are both true.
================
Comment at: clang/lib/Sema/SemaInit.cpp:5941
if (allowObjCWritebackConversion &&
tryObjCWritebackConversion(S, *this, Entity, Initializer)) {
return;
----------------
This use of `Initializer` is also questionable; `tryObjCWritebackConversion()`
will unconditionally dereference it.
================
Comment at: clang/lib/Sema/SemaInit.cpp:5945
if (TryOCLZeroOpaqueTypeInitialization(S, *this, DestType, Initializer))
return;
----------------
This use of `Initializer` is also questionable;
`TryOCLZeroOpaqueTypeInitialization()` will conditionally dereference it.
================
Comment at: clang/lib/Sema/SemaInit.cpp:5976
else
TryUserDefinedConversion(S, DestType, Kind, Initializer, *this,
TopLevelOfInitList);
----------------
This use of `Initializer` looks like it also needs to be protected;
`TryUserDefinedConversion()` unconditionally dereferences it.
================
Comment at: clang/lib/Sema/SemaInit.cpp:6038-6039
TryUserDefinedConversion(S, DestType, Kind, Initializer, *this,
TopLevelOfInitList);
MaybeProduceObjCObject(S, *this, Entity);
----------------
This use of `Initializer` looks like it also needs to be protected;
`TryUserDefinedConversion()` unconditionally dereferences it.
================
Comment at: clang/lib/Sema/SemaInit.cpp:6066-6071
= S.TryImplicitConversion(Initializer, DestType,
/*SuppressUserConversions*/true,
Sema::AllowedExplicit::None,
/*InOverloadResolution*/ false,
/*CStyle=*/Kind.isCStyleOrFunctionalCast(),
allowObjCWritebackConversion);
----------------
`Initializer` is unconditionally dereferenced in
`Sema::TryImplicitConversion()`.
I stopped analyzing other uses here. At this point (at least), it seems clear
that the expectation is that `Initializer` is non-null. That makes me think
that, rather than adding additional checks, we should look for an existing
guarantee that `initializer` is in fact non-null (something that Coverity
missed) or add one. If we need to add such a guarantee, we could add an
`assert(Initializer)` somewhere earlier in the function, but I'm not sure where.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D139148/new/
https://reviews.llvm.org/D139148
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
