[PATCH] D90892: [AIX][FE] Support constructor/destructor attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/CodeGen/CodeGenModule.h:1482
+  void AddGlobalDtor(llvm::Function *Dtor, int Priority = 65535,
+                     bool IsDtorAttrFunc = false);
 
----------------
Xiangling_L wrote:
> 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.
> 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.

The only place that calls `AddGlobalDtor()` without an attribute handy is `AddCXXStermFinalizerToGlobalDtor()` and the only call to that function always passes in the value `65535` (in ItaniumCXXABI.cpp), so passing a null attribute pointer there will behave correctly.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90892/new/

https://reviews.llvm.org/D90892