SWI-Prolog / packages-cpp

The SWI-Prolog C++ interface
29 stars 13 forks source link

ENHANCED: Added SWI-cpp2-flags.h + another blob example #89

Closed kamahen closed 4 days ago

kamahen commented 2 weeks ago

Once this code is reviewed, I'll add the blob example to pl2cpp2.doc

kamahen commented 2 weeks ago

I was thinking of (eventually) adding an interface for options that fitted better with C++. Almost every situation where I've interfaced with foreign library, I've had to deal with options or option flags. So, I thought it would be useful to save some time by providing a class that handled them (I was thinking of something like lookup_{read,write,open}_optdef_and_apply() from rocksdb4pl.cpp). The most useful part is the lookup_list() method; the as_string() is just something I like to use to double-check my code (I've made mistakes in handling options in the past).

But I can easily remove it from the public API for now.

(Speaking of lookup_list ... the code for iterating through a list isn't "obvious", so it might be worth making an API for that, although I'm unsure what form it might take.)

As for CMAKE_C_STANDARD 17, I'm not sure why I did that (it doesn't seem to make a difference for test_ffi.c) ... C-17 is apparently a "bug fix" version, so we should probably require it everywhere. (C23 adds true and false, I think - but requiring that would be too modern?) [I'm not 100% sure about these details - I found some answers on stackoverflow and Wikipedia]

JanWielemaker commented 2 weeks ago

I would be more in favour of keeping this stuff private. If it matures and it turns out a lot of people want it, we can always add it to the public api. I've added too many thing to the public APIs in the past that I later regretted.

As for C11 vs C17, we are happy with C11, so there is no need to enforce C17. It may only cause old compilers to fail. The rule of thumb is to stick with the oldest standard that suits us well enough. So, we only upgrade if we are fairly seriously bothered by the old and typically only of there is no compiler that is still supported around that does not implement the desired standard.

There is a lot more good stuff in C23 (a lot of which is already in gcc and clang) ... We'll probably have to wait a very long time until Microsoft implements it :cry:

kamahen commented 1 week ago

I'm OK with leaving this private; but I think that would require re-doing the PR (I suspect that force-pushes will mess things up). So, it's probably easiest for me to delete this PR and make a new one. Please tell me if you have any other reservations about this code -- if none, I'll also update pl2cpp2.doc with the 2nd blob example.

As for C17 -- if it's a bug-fix, then it might change some "undefined" behaviours, so better to require C17? Do you really want to support obscure compilers that are more than 7 years old?

JanWielemaker commented 1 week ago

Force push should be just fine. And I'm afraid I'm not at all sure all target platforms support c17. It requires gcc 8, which appeared in 2018. Surely various LTS Linux versions are older. So, as long as there is no reason to force that, I see little reason to demand it. Ideally I'd like to say "standard >= 11" as I would like the code to function properly with all recent C standards. That is also why I'm resolving the bool issues. Luckily the forward compatibility is fine as they introduced _Bool in c11 and stdbool.h also long ago, providing the same functionality that becomes truly a language feature with C23.

kamahen commented 1 week ago

I've redone this into 3 separate commits and (I think) incorporated your comments. I've also added the 2nd sample blob code to pl2cpp2.doc

JanWielemaker commented 1 week ago

Thanks. Busy today, but I'll check soon.

kamahen commented 1 week ago

No rush.