Xiangling_L marked 5 inline comments as done. Xiangling_L added inline comments.
================ Comment at: clang/lib/CodeGen/CodeGenModule.h:1482 + void AddGlobalDtor(llvm::Function *Dtor, int Priority = 65535, + bool IsDtorAttrFunc = false); ---------------- aaron.ballman wrote: > Xiangling_L wrote: > > aaron.ballman wrote: > > > There's a fixme comment a few lines up about hardcoding priority being > > > gross and this sort of extends the grossness a bit. Perhaps these > > > functions should accept a `DestructorAttr *`/`ConstructorAttr *` that can > > > be null? > > Yeah, I can understand that putting a magic number as 65535 here is gross, > > but a `bool` with default argument also falls into that way? Or you are > > indicating it's better to not use default argument? > I think the signature should be: > ``` > void AddGlobalDtor(llvm::Function *Dtor, DestructorAttr *DA = nullptr); > ``` > (I don't have strong opinions about whether the attribute pointer should be > defaulted to null or not.) `IsDtorAttrFunc` is implied by a nonnull pointer > and the priority can be gleaned directly from that attribute (or set to the > magic number if the attribute pointer is null). Oh, I see what do you mean here. But the issue is `AddGlobalDtor` is not only used for dtor attribute functions, so we cannot always glean the priority from a `DestructorAttr`. If use `DestructorAttr`, the function signature has to be: ``` void AddGlobalDtor(llvm::Function *Dtor, int Priority, DestructorAttr *DA = nullptr); ``` So that's why I think a `bool` is good enough here. ================ Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:2584 + // We're assuming that the destructor function is something we can + // reasonably call with the default CC. Go ahead and cast it to the + // right prototype. ---------------- aaron.ballman wrote: > Xiangling_L wrote: > > aaron.ballman wrote: > > > Is this assumption safe though given that there are calling convention > > > attributes that can be written on the function? > > Actually I copied this comment from where linux implemented the dtor > > attribute functions. I think it makes sense to make this assumption. > > Because when they are used as destructor functions, they actually don't > > have any caller from source. > Ah, the comment confused me because the user could always write something > like: > ``` > [[clang::stdcall, gnu::destructor]] void func(); > ``` > where the destructor function is not something you can call with the default > (cdecl) calling convention. Should the comment say "we can reasonably call > with the correct CC" instead to avoid this confusion? Sure, that would make more sense under AIX context. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90892/new/ https://reviews.llvm.org/D90892 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
