expose header parameters in clCompileProgram

[Richardk2n]

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

[CLAassistant]

All committers have signed the CLA.

[bashbaug]

Thanks, this is a nice addition!

I think this isn't quite backward compatible though, even with the default arguments. To see why, imagine a case that called compile() and passed a notification function pointer (and possibly some user data) before. This will not work after these changes because the order of the function arguments has changed.

We have several options:

1. Break compatibility. Not ideal, but the odds of this change affecting a significant amont of code is probably low, and if any code breaks the error message and steps to fix it should be clear
2. Move the header arguments to the end of the argument list. This will retain compatibility, although if we did this then the order of arguments will not match clCompileProgram.
3. Keep the current function as-is and add an overload that supports headers. Note, the overload will need to have some non-default arguments for overload resolution to work.

I think my order of preference is (3), then (2), and then (1), though I'm open to alternatives. What do you think?

PS: It would be great to have some unit tests for these changes!

[Richardk2n]

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:

• Should the original function and my header overload just call the second overload, that contains all parameters in order to reduce code?
• Should options be a string instead of char* to also bring this more in line with C++?

[bashbaug]

Thanks, I think this is moving in the right direction, just a few minor things to fix.

I also think it'd be really good to have some tests to ensure this is doing what we want it to do. Is this something you could look into adding? If not, could you please file an issue so we don't lose track of it? Thanks again!

[Richardk2n]

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]

Thanks, this looks really great.

I had a look at adding some tests for this feature and it's going to be complex, since compiling the program also queries the build log. I'm fine with just the tracker issue for now.

[bashbaug]

I left a PR on your repo with some tests - please take a look: Richardk2n#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]

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]

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