pmatos marked 10 inline comments as done.
pmatos added inline comments.
================
Comment at: clang/include/clang/Basic/AddressSpaces.h:59-60
+ wasm_var,
+ wasm_externref,
// This denotes the count of language-specific address spaces and also
----------------
aaron.ballman wrote:
> tlively wrote:
> > What is `wasm_var`? It would be good to have a short comment here (and
> > newline afterward).
> Agreed; I'm not certain why `wasm_var` is part of this changeset.
Yes, it was a mistake during patch splitting. I have removed it. Thanks.
================
Comment at: clang/lib/AST/ASTContext.cpp:2258
+ Width = 0;
\
+ Align = 8; /* ? */
\
+ break;
----------------
tlively wrote:
> I assume things will break if you say 0 here, but would 1 work?
Yes, 1 seems to work. I am not 100% sure but since externref cannot be written
to memory, does it matter?
================
Comment at: clang/lib/AST/ASTContext.cpp:3984
+QualType ASTContext::getExternrefType() const {
+ if (Target->hasFeature("reference-types")) {
+#define WASM_REF_TYPE(Name, MangledName, Id, SingletonId, AS)
\
----------------
tlively wrote:
> Do we need `Target.getTriple().isWasm()` here as well?
Might be useful, in case another target has also a feature called
`reference-types`. :)
================
Comment at: clang/lib/Basic/Targets/DirectX.h:44-45
+ 0, // ptr64
+ 1, // wasm_var
+ 10,// wasm_externref
};
----------------
tlively wrote:
> What are these for? I'm surprised we need to do anything here in the DirectX
> target. Same for the similar lines in other targets.
I was surprised as well, but apparently if you don't add them to all targets,
they will not compile. There's an assignment of these Target specific
AddrSpaceMaps to the AddrSpaceMap: `AddrSpaceMap = &DirectXAddrSpaceMap;`, and
AddrSpaceMap is a LangAS defined in AddressSpaces.h so anything added there
needs to be added to all targets afaiu.
================
Comment at: clang/test/Sema/wasm-refs-and-tables.c:3
+
+// Note: As WebAssembly references are sizeless types, we don't exhaustively
+// test for cases covered by sizeless-1.c and similar tests.
----------------
tlively wrote:
> Should this file be just `wasm-refs.c` since it doesn't do anything with
> tables yet? Same for the next one.
Tables to be added in a future patch. ;)
But yeah, I will rename for now.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D122215/new/
https://reviews.llvm.org/D122215
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
