Anastasia added a comment. In D91531#2406390 <https://reviews.llvm.org/D91531#2406390>, @azabaznov wrote:
> Yes, in general this approach looks good to me conceptually. I have two > suggestions: > > 1. As we discussed, the term //core functionality// should be revisited here. > There's no clear meaning about that in the spec and I think interpreting it > as //supported by default// is a little dangerous. So //core// (AFAIK) means > that it was just promoted to a core specification thus is still remains > optional by targets. Btw I think there is a core and an optional core extension too? I believe that the definition that you are providing applies to the optional core extension but not core extension. However I have created this issue as I think the spec should be explicit about all of these: https://github.com/KhronosGroup/OpenCL-Docs/issues/500. > 1. Sort of a implementation suggestion. I understand that double-scored > identifiers are reserved for any use, but still, can defining such macro as > `__undef_cl_khr_depth_images ` be avoided? We could use `Preproceccor` class > for the things that you are proposing to do. I was trying to do something > similar when implementing features and I tried something like > (`Preprocessor::appendDefMacroDirective` already exists): > > > > UndefMacroDirective *Preprocessor::appendUndefMacroDirective(IdentifierInfo > *II, > SourceLocation Loc) { > if (!II->hasMacroDefinition()) > return nullptr; > UndefMacroDirective *UD = AllocateUndefMacroDirective(Loc); > appendMacroDirective(II, UD); > return UD; > } > > I tried to handle some special pragma in this way and it worked. So maybe > this can be reused without binding to any specific `SourceLocation`? But > maybe there is an other strong concern why > `Preprocessor::appendUndefMacroDirective` still doesn't exist... As far as I can see `Preprocessor::appendDefMacroDirective` is mainly used for the extension https://gcc.gnu.org/onlinedocs/gcc/Push_002fPop-Macro-Pragmas.html. It seems interesting but I am not sure yet if it can help. So what do you plan to do with `Preprocessor::appendUndefMacroDirective`? Perhaps if you give me an example it would help to understand. I agree that `__undef` macro is a bit messy. We could of course make it a bit prettier. For example, (1.) we could add a helper macro #define DEFINE_EXTENSION_MACRO(NAME) \ #if !defined(NAME) && !defined(__undef_ ## NAME)\ #define NAME \ #endif Then we could make a definition of `cl_khr_depth_images` simpler #if defined(__OPENCL_CPP_VERSION__) || (__OPENCL_C_VERSION__ == CL_VERSION_2_0) || \ (__OPENCL_C_VERSION__ >= CL_VERSION_1_2 && defined(__SPIR__) ) DEFINE_EXTENSION_MACRO(cl_khr_depth_images) #endif or (2.) just do something like the following at the end of all extension/feature macro setting code. #if defined(__undef_cl_khr_depth_images) #undef cl_khr_depth_images #endif CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91531/new/ https://reviews.llvm.org/D91531 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
