svenvh added a comment. >> We probably don't want to hold up this patch until we have more thorough >> testing in place, since we don't even have any concrete plans for such >> testing at the moment. > > Well accepting such a significant change that implements the entire > functionality of a new standard without a single test doesn't sound a > workable approach. We should at least add some amount of testing in existing > lib/Headers/opencl-c.h. However, instead of fighting with ad-hoc testing, it > might save us time if we agree on a strategy and just do the testing > properly. After all it is relatively clear what is to be tested. The issue is > mainly with the testing time and amount of overloads to be tested. It should > not be too difficult to guard such testing by a cmake option to avoid long > execution time of default testing target of clang. However, it might be > time-consuming to list all overloads. If we could use C++ templates it would > help.
Perhaps my assumption that we do not want to hold up this patch until we have more thorough testing in place was wrong then. I agree it would be good to have better header testing of the header. I merely wanted to point out that testing the header thoroughly is a substantial piece of work. So if we want to have such testing in place first, we need to delay this patch and start a thread outside of this review. >> One idea for getting some confidence of not breaking OpenCL 2.0 too much, is >> to remove the `-fdeclare-opencl-builtins` flags from the SPIR-V LLVM >> translator <https://github.com/KhronosGroup/SPIRV-LLVM-Translator/> test >> suite and then run those tests to exercise `opencl-c.h` a bit. > > Ok, I think it is generally acceptable. LLVM testing is already very > heterogeneous. Would it mean we can test clang commits with such test > regularly or would it be done once on this patch only? This idea would be a one-off test only, done locally. The aim is to increase confidence in the patch, and only that. This idea does not describe any form of regular testing, nor anything close to cover the entire header. I don't see a trivial way of setting this up for regular testing, as the various translator tests should keep the `-fdeclare-opencl-builtins` flag for speed. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92004/new/ https://reviews.llvm.org/D92004 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
