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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
