Closed MathiasMagnus closed 1 year ago
@keryell can you please check if your comments have been addressed? We'd like to merge this shortly - thanks!
Not sure what's caused the issue with the Windows builds but they do appear to be "real":
D:\a\OpenCL-SDK\OpenCL-SDK\samples\extensions\khr\conway\main.cpp(51,26): error C2397: conversion from 'sf::Style::
' to 'sf::Uint32' requires a narrowing conversion [D:\a\OpenCL-SDK\OpenCL-SDK\build\samples\extensions\khr\conway\conwaycpp.vcxproj]
@bashbaug @keryell I added a few fixes to the tip of the branch, partly to fix what Ben pointed out and addressed some warnings.
The sf:::Uint32
conversion is an SFML API facepalm. sf::Window
takes its style enum flag as an sf::Uint32
, however the enum which supplies the valid values is an anonymous enum. Our cl::sdk::InteropWindow
followed sf::Window
's suit and took the style param as an unsigned integer, but the default underlying type of the anon enum is int
, hence the narrowing conversion.
I changed the API of cl::sdk::InteropWindow
to take the underlying type of the enum in its CTOR and converts it internally before handing it over to sf::Window
. This API change does not leak to consumers, because the SDK lib isn't exported, there we're free to change the API.
GCC also argued about unhandled switch cases, and adding a default
there is safe as we don't intend to handle all cases anyway, just the ones relevant to us.
Motivation
One of the annoying aspects of OpenCL/GL, Vulkan, etc. is the fact that locating assets in general on disk is a pain. Of course large projects solve this using some utility, but toy projects that are still enjoying their dependency-free world are stuck with what the CRT/STL provides if they wish to be portable (a bit like SDK samples).
fopen
andifstream
both interpret the paths relative to the current working directory (CWD), which is unfortunate, because execution success now depends on launching our code from a specific location our code assumes. This assumption definitely blows up in unsuspecting users' faces. C++ luckily has a passable solution:std::filesystem::canonical(argv[0])
which by some magical alignment happens to works on all compilers (CE recently broke execution for MSVC compilers, so those without a working Win env just have to trust me on that it works). Note that even this isn't ideal, because one has to relay theargv
symbol all the way to where this translation occurs. In sample code, that's okay, but more complex programs wantargv
out of thin air. (D2567 targets just this, so maybe in C26 🤞🏻) A mildly elegant solution would be #embed in C23, but mandating a C23 compiler in 2023 is reckless at best. Unfortunately C18 and under doesn't have a similar feature in the CRT, therefore our C samples are subject to CWD relativeness. Seeing this blow up in my students' (and co-lecturers') faces makes me sad. 😞Proposed solution
This problem needs a solution. I cooked up a utility for
OpenCLUtils.lib
. It follows the idiom ofclGetXyzInfo()
functions, first allocate, then return value. It's not dependent onargv
so one may invoke it in the depths of any library init. It wraps whereami, a project with the most permissive dual-license to date: MIT/WTFPLv2. The use of this lib is an implementation detail, it does not leak to consumers of the utility library.The samples in my opinion became much nicer and now they don't fail, regardless of where they're launched from.
Changes introduced
OPENCL_SDK_BUILD_UTILITY_LIBRARIES
CMake optionopencl.hpp
in all regards and make a utility library that is a natural extension to that.