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 = 
+  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.




cfe-commits mailing list

Reply via email to