erichkeane added inline comments.
================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2048
+ QualType Type) {
+ static llvm::DenseMap<QualType, StringRef> Types;
if (Types.empty()) {
----------------
Instead of this initialization, can we make this a constexpr
constructor/collection of some sort and instantiate it inline?
================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2049
+ static llvm::DenseMap<QualType, StringRef> Types;
if (Types.empty()) {
Types[Context.CharTy] = "%c";
----------------
I wonder if instead of this whole business, this function should just be
replaced with a 'switch' on the types.
================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2071
+ if (Types.find(Type) == Types.end())
+ return Types[Context.VoidPtrTy];
+ return Types[Type];
----------------
When can we hit this case? Does this mean that any types we dont understand we
are just treating as void ptrs?
================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2119
+ // But if the array length is constant, we can suppress that.
+ if (llvm::ConstantInt *CL = dyn_cast<llvm::ConstantInt>(Length)) {
+ // ...and if it's constant zero, we can just skip the entire thing.
----------------
This branch doesn't seem right? We are in a `ConstantArrayType`, which means
we DEFINITELY know the length. Additionally, `Length` is constructed directly
from a `llvm::ConstantInt::get`, so we already know this answer.
================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2138
+
+ if (CheckZeroLength) {
+ llvm::Value *isEmpty =
----------------
We already know if this is a 'zero' length array, we should probably just not
emit IR at all in that case.
================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2204
+ if (inArray) {
+ GString = CGF.Builder.CreateGlobalStringPtr("{\n");
+ } else {
----------------
instead of `inArray` you probably should have something a little more
descriptive here, it seems what you really need is something like,
`includeTypeName`, preferably as an enum to make it more clear at the caller
side.
================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2257
+
+ if (CanonicalType->isConstantArrayType()) {
+ GString = CGF.Builder.CreateGlobalStringPtr(
----------------
Do we want to do anything for the OTHER array types?
================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2827
LValue RecordLV = MakeAddrLValue(RecordPtr, Arg0Type, Arg0Align);
- Value *Res = dumpRecord(*this, Arg0Type, RecordLV, Arg0Align,
+ Value *Res = dumpRecord(*this, Arg0Type, RecordLV, Arg0Align, false,
{LLVMFuncType, Func}, 0);
----------------
Typically we do a comment to explain what bool parms mean. Alternatively, if
instead you could create an enum of some sort that would better describe the
'false', it wouldlikely be more helpful. (same on 2252).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D122822/new/
https://reviews.llvm.org/D122822
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
