jgorbe added inline comments.

================
Comment at: clang/lib/AST/ASTContext.cpp:4012-4018
+  if (Target->getTriple().isWasm() && Target->hasFeature("reference-types")) {
+#define WASM_REF_TYPE(Name, MangledName, Id, SingletonId, AS)                  
\
+  if (BuiltinType::Id == BuiltinType::WasmExternRef)                           
\
+    return SingletonId;
+#include "clang/Basic/WebAssemblyReferenceTypes.def"
+  }
+  return QualType();
----------------
aaron.ballman wrote:
> pmatos wrote:
> > aaron.ballman wrote:
> > > Rather than returning a null type, should we assert we're in a wasm 
> > > triple? We shouldn't be trying to form the type outside of a wasm target, 
> > > right?
> > asserting false with message rather than llvm_unreachable is the right way, 
> > correct?
> I'd recommend something like:
> ```
> assert(Target->getTriple().isWasm() && Target->hasFeature("reference-types") 
> && "shouldn't try to generate type externref outsid of WebAssembly target");
> #define WASM_REF_TYPE(Name, MangledName, Id, SingletonId, AS)                 
>  \
>   if (BuiltinType::Id == BuiltinType::WasmExternRef)                          
>  \
>     return SingletonId;
> #include "clang/Basic/WebAssemblyReferenceTypes.def"
>   }
> llvm_unreachable("couldn't find the externref type?");
> ```
> so it's clear that you shouldn't call this outside of a wasm target and that 
> it's not an option to not find the type.
In a non-assert build, doing just `assert(false)` causes the build to fail if 
you use `-Werror -Wreturn-type`.
```
clang/lib/AST/ASTContext.cpp:4097:1: error: non-void function does not return a 
value in all control paths [-Werror,-Wreturn-type]
```


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

Reply via email to