zahiraam added inline comments.
================ Comment at: clang/lib/CodeGen/CodeGenModule.cpp:5006 + if (NeedsGlobalCtor || NeedsGlobalDtor) + DelayedCXXInitPosition[D] = ~0U; + } else { ---------------- efriedma wrote: > zahiraam wrote: > > efriedma wrote: > > > zahiraam wrote: > > > > Do you agree this should be done only when one of those flags is on? > > > Yes, that's fine; I wasn't really paying close attention to the exact > > > code. Just wanted to make the point about the structure of the if > > > statements, and code was the easiest way to explain it. > > > > > > Maybe the outer if statement should actually be `if (isStaticInit(D, > > > getLangOpts()) && NeedsGlobalCtor) {`. > > > > > > On a related note, we might want to avoid the name "ctor", in case that > > > accidentally conflicts with some user code; an "__"-prefixed name would > > > be appropriate. > > >> Maybe the outer if statement should actually be if (isStaticInit(D, > > >> getLangOpts()) && NeedsGlobalCtor) { > > Not sure about that! There are cases where (isStaticInit(D, getLangOpts())) > > = true and NeedsGlobalCtor=false, but NeedsGlobalDtor=true. In which case a > > __dtor needs to be emitted, no? > > > > Writing the condition as you are proposing would actually not get me into > > the body to emit the __dtor. Is that what we want? > EmitCXXGlobalVarDeclInitFunc should be able to handle that case. > > Looking again, I'm a little concerned that in the isStaticInit() case, we're > skipping a bunch of the logic in EmitCXXGlobalVarDeclInitFunc. > EmitCXXCtorInit handles the basic cases correctly, but there are a lot of > special cases in EmitCXXGlobalVarDeclInitFunc. I have left the condition as it was to make sure no cases are left. What other cases are you thinking of? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137107/new/ https://reviews.llvm.org/D137107 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits