Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compilation warnings with PL_initialise #1327

Open
Lecrapouille opened this issue Oct 31, 2024 · 14 comments
Open

Compilation warnings with PL_initialise #1327

Lecrapouille opened this issue Oct 31, 2024 · 14 comments

Comments

@Lecrapouille
Copy link

Hello !

For a C++ Linux project, I do not want to do:

int main(int argc, char **argv)
{
  if ( !PL_initialise(argc, argv) )

but this instead:

...
  char* argv[] = {"", "-q"};
  if ( !PL_initialise(2, argv) )

but the compilo complains: warning: ISO C++ forbids converting a string constant to ‘char*’ [-Wwrite-strings] because of missing const.

  const char* argv[] = {"", "-q"};
  if ( !PL_initialise(argc, argv) )

Now the compilo fails because PL_initialise does not take const char**.

This is the war between serious and modern C++ and legacy C :) Therefore can you adapt your API for:

PL_EXPORT(bool)		PL_initialise(int argc, const char **argv);

Because I have either to choose between warnings and ugly hack char* argv[2] = {const_cast<char*>(""), const_cast<char*>("-q")};

@Lecrapouille
Copy link
Author

You may also be interested by these warnings when including SWI-cpp.h:

/usr/local/lib/swipl/include/SWI-cpp.h: In constructor ‘PlTermv::PlTermv(int)’:
/usr/local/lib/swipl/include/SWI-cpp.h:235:29: warning: conversion to ‘size_t’ {aka ‘long unsigned int’} from ‘int’ may change the sign of the result [-Wsign-conversion]
  235 |   { a0   = PL_new_term_refs(n);
      |                             ^
/usr/local/lib/swipl/include/SWI-cpp.h: In constructor ‘PlTermv::PlTermv(size_t)’:
/usr/local/lib/swipl/include/SWI-cpp.h:243:29: warning: conversion to ‘size_t’ {aka ‘long unsigned int’} from ‘int’ may change the sign of the result [-Wsign-conversion]
  243 |   { a0   = PL_new_term_refs(static_cast<int>(n));
      |                             ^~~~~~~~~~~~~~~~~~~
/usr/local/lib/swipl/include/SWI-cpp.h: In member function ‘PlTerm::operator uint32_t() const’:
/usr/local/lib/swipl/include/SWI-cpp.h:790:12: warning: conversion from ‘int64_t’ {aka ‘long int’} to ‘uint32_t’ {aka ‘unsigned int’} may change value [-Wconversion]
  790 |     return v;
      |            ^

@kamahen
Copy link
Member

kamahen commented Oct 31, 2024

Instead of calling PL_initialise(), I suggest calling Plx_initialise(), which does the error checking for you.
Or, use PlEngine, which also calls PL_cleanup() automatically at the end.

This doesn't fix the problem with const char*, which is a bit more pervasive than just the call to PL_initialise() -- for example, it's also in the constructor for `PlEngine'.

I can change the code for these to use const char* (or overload the code in C++, which might be better) -- @JanWielemaker : what do you think?

@kamahen
Copy link
Member

kamahen commented Oct 31, 2024

SWI-cpp.h is obsolete; I suggest using SWI-cpp2.h (which fixes the size_t problem you pointed out).
There are suggestions for how to convert from SWI-cpp.h to SWI-cpp2.h in the documentation.

@Lecrapouille
Copy link
Author

Ah I saw this file but I did not open it 😱 I'll give you feedback tomorrow. Thank you.

@JanWielemaker
Copy link
Member

Thanks for the feedback. I think it is time to actively deprecate the old SWi-cpp.h by moving the docs to some separate corner or maybe better, remove the docs completely, pointing people at the manual of older versions. @kamahen ?

C++ programmers should have a good experience using SWI-cpp2.h. I'm a little reluctant about consistent introduction of const in SWI-Prolog.h. Yes, it would be nice to have. But, the switch is painful. It requires quite a few updates in many (C) interfaces out there and in my experience such a switch in an API is not painless, in particular if you want your code compatible with the old and new versions. Finally, consistent introduction of const in SWI-Prolog's core implementation is a big task. Unless there is tooling I'm not aware of ...

@Lecrapouille
Copy link
Author

Hi !

  • Yes SWI-cpp2.h looks better point of view C++. For example, in previous version PlEngine may have memory leak:
class PlEngine
{
public:
...

  PlEngine(char *av0)
  { int ac = 0;
    char **av = static_cast<char**>(malloc(sizeof(char *) * 2));   // <=== Allocation but never free !

    av[ac++] = av0;

    if ( !PL_initialise(1, av) )
      throw PlError("failed to initialise");
  }

  ~PlEngine()
  { PL_cleanup(0);
  }
};
  • With SWI-cpp2.h, I still have warnings with unused warnings when compiling g++ -Wall -Wextra (I had to add -Wno-unused-parameter).

  • Concerning const char* it's ok for me to do:

    const char* argv[] = {"", "-q"};
    int argc = sizeof(argv) / sizeof(argv[0]);
    PL_initialise(argc, const_cast<char**>(argv));

while this may lead to undefined behaviour (I hope yo do not modify).

  • Now, I'm stuck with this deprecated warning:
warning: ‘bool PlTerm::operator=(PlTerm) const’ is deprecated: use unify_*() [-Wdeprecated-declarations]
   13 |     args[0] = PlTerm_var();

in this basic example:

    PlCall("consult('test.pl')");
    PlTermv args(1);
    args[0] = PlTerm_var();
    PlQuery query("foo", args);
    while (query.next_solution())
    {
        std::cout << "Result : " << args[0].as_string() << std::endl;
    }

I did not find examples and doc is not clear for beginners.

  • Concerning your C++ API. Point of view of C++ dev. I would have loved to have something hiding a little more guts of Prolog guts. For example, for interfacing queries I'm trying to do something like this:
#include <SWI-cpp2.h>
#include <optional>
#include <stdexcept>
#include <string>
#include <vector>

class PrologInterface
{
public:

    class Exception: public PlException
    {
    public:

        explicit Exception(const PlException& ex) : PlException(ex) {}

        explicit Exception(const std::string& message)
            : PlException(PlTerm(message.c_str()))
        {
        }

        virtual const char* what() const noexcept override
        {
            static std::string msg =
                "Exception: " + PlException::term().as_string();
            return msg.c_str();
        }
    };

    PrologInterface(int argc, char** argv)
    {
        if (!PL_initialise(argc, argv))
        {
            throw Exception("Failed to initialize Prolog engine.");
        }
    }

    PrologInterface()
    {
        const char* argv[] = {"", "-q"};
        int argc = sizeof(argv) / sizeof(argv[0]);

        if (!PL_initialise(argc, const_cast<char**>(argv)))
        {
            throw Exception("Failed to initialize Prolog engine.");
        }
    }

    ~PrologInterface() noexcept
    {
        PL_halt(0);
    }

    void consult(const std::string& file)
    {
        std::string query = "consult('" + file + "')";
        if (!PlCall(query))
        {
            throw Exception("Failed to consult file: " + file);
        }
    }

    template <typename... Args>
    std::vector<std::vector<PlTerm>> query(const std::string& query_str, Args&&... args)
    {
        PlTermv termv(sizeof...(args));
        setArguments(termv, std::forward<Args>(args)...);

        PlQuery query(query_str.c_str(), termv);
        std::vector<std::vector<PlTerm>> solutions;

        try
        {
            while (query.next_solution())
            {
                std::vector<PlTerm> solution;
                for (size_t i = 0; i < termv.size(); ++i)
                {
                    solution.push_back(termv[i]);
                }
                solutions.push_back(solution);
            }
        }
        catch (const PlException& ex)
        {
            throw Exception(ex);
        }

        return solutions;
    }

private:

    template <typename Arg, typename... Args>
    void setArguments(PlTermv& termv, Arg&& arg, Args&&... args)
    {
        termv[sizeof...(Args)] = PlTerm(std::forward<Arg>(arg));
        if constexpr (sizeof...(Args) > 0)
        {
            setArguments(termv, std::forward<Args>(args)...);
        }
    }

    void setArguments(PlTermv&) {}
};

Idea:

    auto solutions = prolog.query("foo", "X");
    if (solutions.empty())
    {
        std::cout << "No solutions found" << std::endl;
    }
    else
    {
        for (const auto& solution : solutions)
        {
            for (const auto& term : solution)
            {
                std::cout << term.as_string() << " ";
            }
            std::cout << std::endl;
        }
    }

But this code does not compile, and I guess has several issues: "X" shall be PlTerm_var() and plenty of things that I noob like me forbid :)

@kamahen
Copy link
Member

kamahen commented Nov 1, 2024

About char* argv[] ... it seems that the standard says that the signature for main() does not have a const (for legacy reasons).

I tried calling PlEngine() with both a char** and char*[] and they both worked (-Wpedantic -Wwrite-strings). I think that your problem is that const char*[] just doesn't work in this situation and the C/C++ standard is unlikely to change (I've added a test case to test_cpp.cpp). const_cast doesn't seem to help here either; and there also seem to be overloading problems if I add PlEngine(int argc, const *argc[]). (It appears that I'd need a reinterpret_cast, which seems strange ...)

I'll look at your other comments later. There are some legacy issues (both in SWI-Prolog and C++), so I'm not sure what's best.

@JanWielemaker - I'm in favour of removing SWI-cpp.h and its documentation, especially as we've seen that its presence can confuse people. I occasionally refer back to it, to understand why some "feature" of SWI-cpp2.h is the way it is; perhaps move SWI-cpp.h and its documentation to a sub-directory deprecated, to allow old code to continue to work by a simple change to the #include? (And perhaps add #warning - is there a portable way of outputting a compile-time warning?)

@Lecrapouille
Copy link
Author

  • Yes, as extra step, instead of #warning you can add #error "Depracted. Use SWI-cpp2.h instead" in SWI-cpp.h without removing the file from the git. If people need them, they can comment the #error.

  • PlEngine(int argc, char **argv) does not have warnings by itself. That just force to do some alloc like you did in https://github.com/SWI-Prolog/packages-cpp/blob/61fd1c7d7c77559a6826755b9c6831f9bc6fb6fc/SWI-cpp.h#L1277 with possible missing free calls. C++ warn you against /*const*/ char* argv[] = {"", "-q"}; since "-q" shall go to the text section by the linker and you should not modify it. I know this is an old war between saggy posix functions and modern C++. I remembered nightmares with exec* functions having the same issue.

No worry, this is just a minor issue.

If the doc could be a little more explicit about PlTermv args(1); args[0] = ... and showing what to replace, it would be great. And thank you for making Prolog alive. We probably include Prolog inside a professional C++ project (at least for prototyping).

@kamahen
Copy link
Member

kamahen commented Nov 1, 2024

I just tried some of the example code that uses PlEngine in the documentation for PlTerm_tail and it didn't work. I'll try to fix this (and other documentation) "soon".

@JanWielemaker
Copy link
Member

  • Yes, as extra step, instead of #warning you can add #error "Depracted. Use SWI-cpp2.h instead"

That is a it too drastic IMO. I don't want to break working code if not strictly necessary and having to edit in a distributed software package is not what I like doing (and one may not have the rights). A warning and removing the docs should be enough.

@JanWielemaker
Copy link
Member

I just deleted the version 1 docs and uncommented the #warning for the deprecation. Probably some more work needs to be done to the docs to make the status completely clear, but this seems a step ahead.

@kamahen
Copy link
Member

kamahen commented Nov 2, 2024

If someone is converting old code to use SWi-cpp2.h (especially if they didn't write the code), they might need the old documentation to understand some things, such as the use of cast (although this is mentioned in the summary of changes).
On the other hand, removing the documentation avoids a source of confusion.

@Lecrapouille
Copy link
Author

Just for information: I also found this nice repo https://github.com/ethz-asl/ros-prolog (sadly archived! Why ?) with a smooth C++ API wrapping your C API. It also hides some Prolog guts for example where a term is either a variable or an atom. I'm personally not fan of projects using Pimpl (i.e. they use void* instead of PL_engine_t) in our case to avoid exporting the SWI-Prolog.h.

I'm giving a try cleaning on folders prolog_swi and prolog_common (other code can be drop I guess):

  • replacing boost:: by std::
  • replacing ROS exception by std exception
  • replacing ROS stream by std::cout/cerr
  • Define SWIPL_EXECUTABLE

The only dark point is the compilation of the Engine class: they are using old Prolog PL_thread_attr_t https://github.com/ethz-asl/ros-prolog/blob/9f190eab986e9523d26e7c332ac66c18cd0805fd/prolog_swi/src/Engine.cpp#L81-L87

@JanWielemaker
Copy link
Member

I also found this nice repo https://github.com/ethz-asl/ros-prolog

I guess this is more a discussion for the Discourse forum. There are definitely more ways to talk to Prolog from C++. I guess the main advantage of using the bundled one is that you can be fairly sure it will be maintained for a long time. I'm not much of a C++ programmer though and have little opinion on this ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants