FirebirdSQL / firebird

Firebird server, client and tools
https://www.firebirdsql.org/
1.23k stars 213 forks source link

Add generated files for OO API for C language to distribution #8197

Closed sim1984 closed 2 days ago

sim1984 commented 1 month ago

There are many programs written in C. At the moment they use the legacy isc API. But at the moment the isc API is not being developed and it is impossible to add new functionality to such programs. I know that with the help of CLOOP we can generate an OO API for the C language. But most developers are not aware of this. Moreover, they are afraid if some generated C interfaces get into their program, which over time can change in FB and they will become broken.

There are real examples of this need. Recently I made a patch for Firebird 4.0 data type support for the pdo_firebird driver for PHP. I simply bound all the new data types to strings, since PHP has no other types in which the values ​​of the new FB types could be placed. And it works. With one exception, for TIME[STAMP] TZ data types, you cannot set the display format. This is easily solved in the OO API, but PHP is written in C, and it uses the isc API. In my opinion, it is quite strange to include two generated files fb_api.h and fb_impl_api.c in the pdo_firebird source code to solve this problem. I think maintainers will simply not accept such a patch, because the files are generated and may violate their coding standards. It would be a different matter if they were included in the Firebird installation and were located in ${fb_root}/include like other API files.

AlexPeshkoff commented 1 month ago

С generator was not tested on real data for rather long time. I will take a look at it but this may be not as trivial as adding files to distro.

BTW:

Moreover, they are afraid if some generated C interfaces get into their program, which over time can change in FB and they will become broken.

Is there an example of backwards-incompatible change of interfaces in released versions?

sim1984 commented 1 month ago

Is there an example of backwards-incompatible change of interfaces in released versions?

I don't remember any, but that's not the main problem. The main problem is that the lack of ready-made files greatly complicates the implementation of new features in projects written in C.

I made a mini-example of using the C OO API. It's certainly not ideal, but it just helped me figure out how it works. https://github.com/sim1984/fb_c_oo_api_example

After I figured it out, I made a patch for PHP pdo_firebird https://github.com/php/php-src/pull/15230

In this form, it looks dubious, but it is even more difficult to justify the need to add two files to the project that seem alien to the project.

If cloop were at least enabled in the Firebird build, then third-party builds could be configured to automatically generate these files. But cloop is only in the source files, it is not documented, and it does not even have help on command line options. That is, in fact, it is more of a tool for Firebird developers, and not for developers who want to generate Firebird API for their programs.

My wishes consist of three points:

  1. The cloop tool should be included in Firebird builds. It should have help on command line options.
  2. It would be desirable to have a ready-made OO API file for the C language, as it is done for C++ and Pascal.
  3. It would be nice to make at least one example of working with the OO API in C.
AlexPeshkoff commented 1 month ago

On 7/26/24 09:20, Simonov Denis wrote:

There are many programs written in C. At the moment they use the legacy isc API. But at the moment the isc API is not being developed and it is impossible to add new functionality to such programs. I know that with the help of CLOOP we can generate an OO API for the C language. But most developers are not aware of this. Moreover, they are afraid if some generated C interfaces get into their program, which over time can change in FB and they will become inoperable.

There are real examples of this need. Recently I made a patch for Firebird 4.0 data type support for the pdo_firebird driver for PHP. I simply bound all the new data types to strings, since PHP has no other types in which the values ​​of the new FB types could be placed. And it works. With one exception, for |TIME[STAMP] TZ| data types, you cannot set the display format. This is easily solved in the OO API, but PHP is written in C, and it uses the isc API. In my opinion, it is quite strange to include two generated files |fb_api.h| and |fb_impl_api.c| in the pdo_firebird source code to solve this problem. I think maintainers will simply not accept such a patch, because the files are generated and may violate their coding standards. It would be a different matter if they were included in the Firebird installation and were located in include like other API files.

Denis, one more question. Do you see big need to include source file fb_impl_api.c into include directory? For me building of static library (and having it in default library path) looks much more logical. What do you think?

sim1984 commented 1 month ago

The presence of fb_impl_api.c into include directory is really illogical. There are two options: either a static library, or declare these functions built-in.

If you use a static library, then in theory you can hide the details of all structures.

AlexPeshkoff commented 4 weeks ago

I've decided to not add cloop binary to distribution - with ready-for-use plain-C API files it makes no sense. @sim1984 - please review the result, after it suppose I can backport it to FB5.

sim1984 commented 4 weeks ago

Ok. The only remark is that I didn't see lib/fb_c_api_ms.lib generated for msvc, but it seems that this is not for you.

I'll try to check C api under Linux tonight.

sim1984 commented 4 weeks ago

In C, for functions without parameters, it is advisable to write void inside the brackets.

struct IMaster* ISC_EXPORT fb_get_master_interface(void);
sim1984 commented 4 weeks ago
#include <ibase.h>

struct IMaster* ISC_EXPORT fb_get_master_interface(void);

/* This file was autogenerated by cloop - Cross Language Object Oriented Programming */

#ifndef FB_C_API_H
#define FB_C_API_H

I think declaring the function fb_get_master_interface and include before the header guard is not very good.

AlexPeshkoff commented 4 weeks ago

But that reflects what happens: the remainder of the file really was autogenerated, but header before this line added manually later.

sim1984 commented 4 weeks ago

For C++ we have a file Interface.h which includes an autogenerated file IdlFbInterfaces.h. Maybe we should do the same for C?

AlexPeshkoff commented 4 weeks ago

That can be done - but what benefits does it have?

dyemanov commented 1 week ago

@AlexPeshkoff, do you still plan to backport it into v5?

AlexPeshkoff commented 1 week ago

First of all I wait for confirmation from Denis that generated files are ok.

sim1984 commented 1 week ago

My test example compiled and ran successfully on Linux. However, I see two shortcomings:

  1. struct IMaster* ISC_EXPORT fb_get_master_interface(void); is placed before the header guard. This can create problems if fb_c_api.h is included more than once.
  2. Under Windows, there is no fb_c_api_ms.lib and header file
sim1984 commented 1 week ago

Good

AlexPeshkoff commented 1 week ago

@dyemanov Dmitry, what do you think - do we need this support for windows? If yes - can you take care about it?

dyemanov commented 1 week ago

It should be available for all platforms. As for Windows - do you mean just the packaging (include files into binary kits) or something more is required?

sim1984 commented 1 week ago

It should be available for all platforms. As for Windows - do you mean just the packaging (include files into binary kits) or something more is required?

Not just the packaging. The static link library fb_c_api_ms.lib is also needed

AlexPeshkoff commented 1 week ago
  1. Generate .c & .h files from idl
  2. Build static library from generated .c
  3. Package .h and .lib files
aafemt commented 1 week ago

The static link library fb_c_api_ms.lib is also needed

Not needed. Actual import library for installed MSVC version can be generated with lib utility while GCC support linking directly with DLL.

AlexPeshkoff commented 1 week ago

@aafemt Not in this case. fb_c_api is not a stub for dll, it's real (yep, trivial) statically linked code.

sim1984 commented 1 week ago

The static link library fb_c_api_ms.lib is also needed

Not needed. Actual import library for installed MSVC version can be generated with lib utility while GCC support linking directly with DLL.

You can generate anything. You can even download the cloop source code and generate everything. The ticket is about not doing this, because those who need to work with the API are afraid to generate something on their own. They think that since there are no ready-made files that can be used right away, there is no way. Those who understand how to generate an API do not need all this, but they are a minority. In addition, CLOOP is not distributed as part of the distribution.

aafemt commented 1 week ago

Are MSVC object files compatible between versions? Are you going to provide a library version for GCC as well?

sim1984 commented 1 week ago

We are already doing this. Look in the lib folder of the Windows distribution. There are fbclient_ms.lib and ib_util_ms.lib. It would be logical to do the same with the new static library.

aafemt commented 1 week ago

These are just import library which are generated by lib utility. And according to my experience they often refuse linking unless you re-generate them using your actual utility version.

sim1984 commented 1 week ago

Are you suggesting to make another DLL and import from it? Personally, I have not noticed any problems with lib. Probably because only MSVC is used for compilation under Windows.

aafemt commented 1 week ago

Compilation of the Firebird itself and compilation of a user application using Firebird are two different things. I wonder why Alex decided to provide hand-written envelopes instead of direct calls to vtable members.

asfernandes commented 1 week ago

My original idea with this .c file would be simple as that one would include it once in their program. Include a .c file? Yes! Simple. Now you see the problems with the elegant solution.

aafemt commented 1 week ago

C-way is macros. So not

CLOOP_EXTERN_C int IReferenceCounted_release(struct IReferenceCounted* self)
{
    return self->vtable->release(self);
}

but #define IReferenceCounted_release(self) self->vtable->release(self) and this is in a normal .h file.

AlexPeshkoff commented 1 week ago

C-way is macros. So

define IReferenceCounted_release(self) return self->vtable->release(self);

I like this suggestion. Objections?

sim1984 commented 1 week ago

If the macros are formatted correctly, i.e. don't miss any deep errors like

#define fn(a, b) a * b
fn(5 + 7, 6 + 8);

then it will do just fine.

aafemt commented 1 week ago

Out of curiosity: guys, have you ever looked into MS headers for any COM object?..

AlexPeshkoff commented 1 week ago

If the macros are formatted correctly, i.e. don't miss any deep errors like


#define fn(a, b) a * b

Certainly it should be #define fn(a, b) ((a) * (b))

fn(5 + 7, 6 + 8);



then it will do just fine.
sim1984 commented 5 days ago

I tried to build and run the example with the new fb_c_api.h and everything looks good to me now. I think it can be ported to Firebird 5.0.