hubert.reinterpretcast marked an inline comment as done.
hubert.reinterpretcast added inline comments.
================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:700
+
+ Fn = CreateGlobalInitOrDestructFunction(
+ FTy, llvm::Twine("__sterm80000000_clang_") + GlobalUniqueModuleId, FI,
----------------
Xiangling_L wrote:
> hubert.reinterpretcast wrote:
> > The called function is to be renamed.
> Any suggestions here about how to represent the functionality of `__sterm`
> and `_GLOBAL__D_a` into one word? Cannot think of a good name...
>
> Actually I am wondering do we need to rename the `Destruct` part? The
> `__sterm` and `_GLOBAL__D_a` do destruct the object by calling sterm
> finalizers and object destructors?
Being clear in the naming of the function helps to signal its purpose to future
maintainers. The sterm finalizers are unlikely to be understood from `Destruct`
(it implies plain destruction a bit too much). Maybe "cleanup"?
================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:807
void CodeGenFunction::GenerateCXXGlobalDtorsFunc(
llvm::Function *Fn,
----------------
Xiangling_L wrote:
> hubert.reinterpretcast wrote:
> > This function is to be renamed.
> I will try `GenerateCXXGlobalDestructFunc` based on the thoughts as I also
> mentioned elsewhere that the `__sterm` and `_GLOBAL__D_` function do destruct
> the object by calling sterm finalizers and object destructors.
>
> Any suggestions or concerns about this renaming? Or do we really need to
> rename this one?
I think "cleanup" works here too.
================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:639
+ if (CXXGlobalInits.empty())
+ return;
----------------
Xiangling_L wrote:
> hubert.reinterpretcast wrote:
> > Can this part be committed in a separate patch? It does not appear to have
> > dependencies on other parts of this patch and has the appearance of being a
> > possible change for other platforms.
> Sure. I will create a NFC patch for this part and try to commit it first. But
> so far, I think I can keep this part in this patch just for review purpose to
> make the patch look nicer?
Sure; you'd need the new patch to exist before rebasing on it anyway. We can
leave the rebasing to the commit or later in the review.
================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:4467
+
+ // Create a variable destruction function.
+ const CGFunctionInfo &FI = CGM.getTypes().arrangeNullaryFunction();
----------------
Xiangling_L wrote:
> hubert.reinterpretcast wrote:
> > Suggestion:
> > Create the finalization action associated with a variable.
> I don't get your point, can you elaborate on this a little bit?
This is my suggestion for the comment (to replace "Create a variable
destruction function").
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D74166/new/
https://reviews.llvm.org/D74166
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
