svenvh added inline comments.
================ Comment at: clang/test/SemaOpenCL/fdeclare-opencl-builtins.cl:38 +typedef uint cl_mem_fence_flags; +#define CLK_GLOBAL_MEM_FENCE 0x02 + ---------------- Anastasia wrote: > Should we add this conditionally if the base header is not included? > > In the subsequent patches where you will add other functions, we should make > sure that the base header indeed contains the declarations we use in tablegen. > Should we add this conditionally if the base header is not included? It is already inside an `#ifdef NO_HEADER`, see line 21. > In the subsequent patches where you will add other functions, we should make > sure that the base header indeed contains the declarations we use in tablegen. Not sure what you mean... When we test a builtin function that relies on an enum or typedef from the base header, Sema won't succeed if the enum or typedef isn't available. ================ Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:727 + } + return S.Context.getEnumType(Result.getAsSingle<EnumDecl>()); +} ---------------- Anastasia wrote: > I think we should add an assert that Result.getAsSingle<EnumDecl>() indeed > holds. Consider if instead of using base header the types are defined > manually and they are regular integers, not enums. That is a good point. I think it would be even better to handle such a case properly with a diagnostic, and not rely on just an assert. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96051/new/ https://reviews.llvm.org/D96051 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
