yihanaa added a comment.
Thanks for your suggestion @erichkeane !
================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2048
+ QualType Type) {
+ static llvm::DenseMap<QualType, StringRef> Types;
if (Types.empty()) {
----------------
erichkeane wrote:
> Instead of this initialization, can we make this a constexpr
> constructor/collection of some sort and instantiate it inline?
> Instead of this initialization, can we make this a constexpr
> constructor/collection of some sort and instantiate it inline?
I don't have a good idea because it relies on Context to get some types like
const char *
================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2071
+ if (Types.find(Type) == Types.end())
+ return Types[Context.VoidPtrTy];
+ return Types[Type];
----------------
erichkeane wrote:
> When can we hit this case? Does this mean that any types we dont understand
> we are just treating as void ptrs?
The previous version treated it as a void *ptr for unsupported types, such as
arrays (before this patch) and __int128. This is the case i saw in an issue. I
also think that for unsupported types, we should generate a clearer message
saying "This type is temporarily not supported", do you have any good ideas?
================
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.
----------------
erichkeane wrote:
> 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.
I agree with you
================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2138
+
+ if (CheckZeroLength) {
+ llvm::Value *isEmpty =
----------------
erichkeane wrote:
> We already know if this is a 'zero' length array, we should probably just not
> emit IR at all in that case.
Good idea!
================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2204
+ if (inArray) {
+ GString = CGF.Builder.CreateGlobalStringPtr("{\n");
+ } else {
----------------
erichkeane wrote:
> 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.
I think that's a good idea for clarity.
================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2257
+
+ if (CanonicalType->isConstantArrayType()) {
+ GString = CGF.Builder.CreateGlobalStringPtr(
----------------
erichkeane wrote:
> Do we want to do anything for the OTHER array types?
I understand struct fields must have a constant size, and I see" 'variable
length array in structure' extension will never be supported" from clang's
diagnostics
================
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);
----------------
erichkeane wrote:
> 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).
I agree!
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
