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