Xiangling_L added inline comments.
================
Comment at: clang/lib/AST/ItaniumMangle.cpp:5217
+ else
+ Mangler.getStream() << D->getName();
+}
----------------
jasonliu wrote:
> If I understand correctly, this function will come in pair with
> `__cxx_global_var_init`.
> `__cxx_global_var_init` use number as suffix in the end to avoid duplication
> when there is more than one of those, but we are using the variable name as
> suffix here instead.
> Do we want to use number as suffix here to match what `__cxx_global_var_init`
> does? It would help people to recognize the pairs and make them more
> symmetric.
This is somewhere I am kinda on the fence. Personally, I think embed decl name
in the __cxx_global_var_destruct_ / __cxx_global_vat_init_ as
`mangleDynamicAtExitDestructor` does is more helpful for the user to debug the
static init.
I am not sure if we want to give up that benefit and be consistent with current
`__cxx_global_vat_init_ ` using number suffix or do we want to replace number
suffix by decl name for `__cxx_global_vat_init_ ` as well.
================
Comment at: clang/lib/CodeGen/CGCXXABI.h:113
+
+ virtual bool isCXXGlobalInitAndDtorFuncInternal() const { return true; }
+
----------------
jasonliu wrote:
> Do we need this isCXXGlobalInitAndDtorFuncInternal? Looks like it's always
> going to return ! useSinitAndSterm() result.
AFAIK, not all platforms which use sinit and sterm have external linkage
sinit/sterm functions. And also since for 7.2 AIX we are considering change
sinit/sterm to internal linkage symbols, I seperate this query out.
================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:305
+ if (llvm::Function *unatexitFn =
+ dyn_cast<llvm::Function>(unatexit.getCallee()))
+ unatexitFn->setDoesNotThrow();
----------------
jasonliu wrote:
> Is there a valid case that unatexit.getCallee() returns a type which could
> not be cast to llvm::Function?
> i.e. Could we use cast instead of dyn_cast?
I used `cast` instead of `dyn_cast` before Diff 9 actually, and then I noticed
that `clang-tidy` gave error and asked me to use `dyn_cast` instead. Cannot
recall what the error says though...
================
Comment at: clang/lib/CodeGen/CodeGenModule.h:20
#include "SanitizerMetadata.h"
+#include "clang/AST/Attr.h"
#include "clang/AST/DeclCXX.h"
----------------
jasonliu wrote:
> Is this Attr.h needed somewhere in this file?
Oops..this is something I forgot to remove. Good catch!
================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:4481
+
+ llvm::BasicBlock *DestructCheckBlock =
CGF.createBasicBlock("destruct.check");
+ llvm::BasicBlock *EndBlock = CGF.createBasicBlock("destruct.end");
----------------
jasonliu wrote:
> I think we need a better naming for this and make it consistent with the end
> block below.
> As it is for now, `destruct.check` is confusing, as we are not checking
> anything here and we are just going to call destructor directly in this
> block.
Thanks, I will try `destruct.call` instead.
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D74166/new/
https://reviews.llvm.org/D74166
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
