KhronosGroup / OpenCL-CLHPP

Khronos OpenCL-CLHPP
Apache License 2.0
375 stars 130 forks source link

expose header parameters in clCompileProgram #286

Closed Richardk2n closed 6 months ago

Richardk2n commented 9 months ago

I use this to include dynamically generated headers. Exposing this should offer more flexibility without breaking existing code due to the defaults.

CLAassistant commented 9 months ago

CLA assistant check
All committers have signed the CLA.

Richardk2n commented 8 months ago

You are right of course about compatibility. Not sure what inhibited my thinking when I wrote that.

Due to the limitation of the non default argument needing to come first and me wanting to preserve argument order, this leads to the requirement to provide options, which is ok I guess. No need to break compatibility.

While I was at it, I also wrote an overload addressing #285

Honestly, I have no clue how to write unit tests for something like this.

Questions:

Richardk2n commented 7 months ago

I fully agree, that tests would be nice. I looked into it and could not figure it out (sorry). Consequently, I wrote an issue. Otherwise, I made the changes as advised.

bashbaug commented 7 months ago

I left a PR on your repo with some tests - please take a look: https://github.com/Richardk2n/OpenCL-CLHPP/pull/1

I do wonder if we should re-think passing the options as a string vs. a const char*, for consistency and to allow passing nullptr options.

Richardk2n commented 7 months ago

In my opinion using string is preferable. I find "" to be more intuitive than nullptr but I do acknowledge this is personal preference. Using string shortens code in programs using these headers and importantly provides consistency between the arguments of the function. I would rather add a string overload for the remaining compile function (if this is possible) and deprecate the const char* one. I prefer it this way, but I do need the function, so if you insist I will change it to get this merged.

bashbaug commented 6 months ago

Merging as discussed in the April 9th teleconference - thanks!