hubert.reinterpretcast added inline comments.
================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:293
+CodeGenFunction::unregisterGlobalDtorWithUnAtExit(llvm::Function *dtorStub) {
+ // extern "C" int unatexit(void (*f)(void));
+ assert(dtorStub->getFunctionType() ==
----------------
Please add a comment explaining the meaning of the zero and non-zero return
values.
================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:603
if (!PrioritizedCXXGlobalInits.empty()) {
+ assert(!UseSinitAndSterm && "Prioritized Sinit and Sterm functions are not"
+ " supported yet.");
----------------
I don't think we should capitalize "sinit" and "sterm" in comments. Indeed,
there's a comment below where they are not capitalized.
================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:639
+ if (CXXGlobalInits.empty())
+ return;
----------------
Can this part be committed in a separate patch? It does not appear to have
dependencies on other parts of this patch and has the appearance of being a
possible change for other platforms.
================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:641
- for (size_t i = 0; i < FileName.size(); ++i) {
- // Replace everything that's not [a-zA-Z0-9._] with a _. This set happens
- // to be the set of C preprocessing numbers.
- if (!isPreprocessingNumberBody(FileName[i]))
- FileName[i] = '_';
- }
+ // Include the filename in the symbol name. When not using sinit and sterm
+ // functions, including "sub_" matches gcc and makes sure these symbols
----------------
The first sentence does not apply unconditionally in the status quo of this
patch. Perhaps keep the original comment completely with something like:
```
// When not using sinit and sterm functions:
// ...
```
================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:682
void CodeGenModule::EmitCXXGlobalDtorFunc() {
if (CXXGlobalDtors.empty())
return;
----------------
Following from my previous comments on `CXXGlobalDtors`, I am not convinced
that `__sterm` functions match the role.
================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:704
AddGlobalDtor(Fn);
+ CXXGlobalDtors.clear();
}
----------------
If this is another drive-by fix, can we put it in a separate patch?
================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:817
std::tie(CalleeTy, Callee, Arg) = DtorsAndObjects[e - i - 1];
- llvm::CallInst *CI = Builder.CreateCall(CalleeTy, Callee, Arg);
+ llvm::CallInst *CI = (Arg == nullptr)
+ ? Builder.CreateCall(CalleeTy, Callee)
----------------
This change would not belong in this function if its scope remains the
generation of a "GlobalDtorsFunc" (based on my understanding of the latter).
================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:4445
+ if (D.getTLSKind() != VarDecl::TLS_None)
+ llvm::report_fatal_error("thread local storage unimplemented on AIX yet");
+
----------------
s/unimplemented on AIX yet/not yet implemented on AIX/;
================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:4467
+
+ // Create a variable destruction function.
+ const CGFunctionInfo &FI = CGM.getTypes().arrangeNullaryFunction();
----------------
Suggestion:
Create the finalization action associated with a variable.
================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:4480
+
+ llvm::Value *NeedsDestruct = CGF.Builder.CreateIsNull(V, "guard.hasDtor");
+
----------------
I don't think this is a real "guard variable". I think the comments need to
explain this (including scare quotes for "guard variable".
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D74166/new/
https://reviews.llvm.org/D74166
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
