Xiangling_L marked 2 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: > > > 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. The reason why `AddCXXStermFinalizerToGlobalDtor() ` currently always pass in 65535 is because priority hasn't been supported by AIX yet(You can find the assertion few lines above there). And that would happen in the near future. The same thing happens in function `EmitCXXGlobalCleanUpFunc()`, after we support init priority, we won't always use default value. 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
