firebird icon indicating copy to clipboard operation
firebird copied to clipboard

Add version definition (#define) in public interfaces for conditional compilation

Open asfernandes opened this issue 1 year ago • 16 comments

asfernandes avatar Oct 08 '22 18:10 asfernandes

This should make #6974 obsolete.

asfernandes avatar Oct 08 '22 18:10 asfernandes

Version constants should be used in places like if (cloopVTable->version < 4) and static const unsigned VERSION = 2;

AlexPeshkoff avatar Oct 10 '22 08:10 AlexPeshkoff

Public API changes should be applied in sync to other, non-C++, cloop generators.

AlexPeshkoff avatar Oct 10 '22 08:10 AlexPeshkoff

Version constants should be used in places like if (cloopVTable->version < 4)

No, this is about previous versions too, while the #define is about he latest only.

and static const unsigned VERSION = 2;

I also disagree as it should be the same number and it's generated code.

I don't like to see C++ code depending on preprocessor.

asfernandes avatar Oct 10 '22 10:10 asfernandes

Public API changes should be applied in sync to other, non-C++, cloop generators.

Do you mean Pascal?

C generator already put this, in a slight different format (without namespace).

I prefer to prefix with namespace in C++.

asfernandes avatar Oct 10 '22 10:10 asfernandes

I'm thinking in change name pattern from FIREBIRD_IPluginSet_VERSION to FIREBIRD_IPLUGIN_SET_VERSION.

What do you think?

asfernandes avatar Oct 10 '22 10:10 asfernandes

I am also thinking in generating per-method define like this:

#define FIREBIRD_IPLUGIN_SET_HAS_GET_NAME
#define FIREBIRD_IPLUGIN_SET_HAS_GET_MODULE_NAME
#define FIREBIRD_IPLUGIN_SET_HAS_GET_PLUGIN

So code does not need to use version numbers to detect features.

asfernandes avatar Oct 10 '22 10:10 asfernandes

I am also thinking in generating per-method define like this:

IMHO they are pointless and only pollute headers.

aafemt avatar Oct 10 '22 10:10 aafemt

I am also thinking in generating per-method define like this: IMHO they are pointless and only pollute headers.

There is no easy way to know necessary numbers to compare against the version constants.

asfernandes avatar Oct 10 '22 10:10 asfernandes

Looking to interface definition with the constant nearby is easy enough for me.

aafemt avatar Oct 10 '22 10:10 aafemt

I am also thinking in generating per-method define like this:

IMHO they are pointless and only pollute headers.

+1

hvlad avatar Oct 10 '22 11:10 hvlad

On 10/10/22 13:44, Adriano dos Santos Fernandes wrote:

Version constants should be used in places like
|if (cloopVTable->version < 4)|

No, this is about previous versions too, while the #define is about he latest only.

ok

and
|static const unsigned VERSION = 2;|

I also disagree as it should be the same number and it's generated code.

I don't like to see C++ code depending on preprocessor.

I dislike same literal constants used more than once and see no problems with reasonable use of preprocessor with C++ (boost?).

AlexPeshkoff avatar Oct 10 '22 11:10 AlexPeshkoff

On 10/10/22 13:45, Adriano dos Santos Fernandes wrote:

Public API changes should be applied in sync to other, non-C++,
cloop generators.

Do you mean Pascal?

Yes, and I know that you have java generator. BTW, no idea why is it still missing in FB tree.

AlexPeshkoff avatar Oct 10 '22 11:10 AlexPeshkoff

On 10/10/22 13:47, Adriano dos Santos Fernandes wrote:

I'm thinking in change name pattern from |FIREBIRD_IPluginSet_VERSION| to |FIREBIRD_IPLUGIN_SET_VERSION|.

What do you think?

+1

AlexPeshkoff avatar Oct 10 '22 11:10 AlexPeshkoff

Yes, and I know that you have java generator. BTW, no idea why is it still missing in FB tree.

Because cloop concept was to be a general solution, not only specific to Firebird, and we were unable to write a generic tool with Firebird restrictions: no package managers, no libraries, etc.

So each feature was added in different trees.

asfernandes avatar Oct 10 '22 11:10 asfernandes

I dislike same literal constants used more than once and see no problems with reasonable use of preprocessor with C++ (boost?).

Using the dump approach of requiring inspection of generated code to know about versions like others mentioned to want, yes, it may make sense to use the define in the const to make user's life less hell.

asfernandes avatar Oct 10 '22 11:10 asfernandes

Do you mean Pascal? Yes, and I know that you have java generator. BTW, no idea why is it still missing in FB tree.

Accordingly to https://www.freepascal.org/docs-html/prog/progsu11.html#x18-170001.2.11 $DEFINE are not exported in the unit.

So it would have no use AFAIU.

Do you know how to output an outside usable define?

asfernandes avatar Oct 18 '22 10:10 asfernandes

Generate separate file with version definitions that may be included by people who needs version information in pascal? (but take into an account - I do not pretend to be pascal professional, may be something better can be done)

AlexPeshkoff avatar Oct 18 '22 11:10 AlexPeshkoff

The pascal stuff is in wrong place. It should be maintained outside core, with contributions from people who understand it.

asfernandes avatar Oct 18 '22 11:10 asfernandes

The problem is that pascal API is already actively used by people. On the other hand nobody requested interfaces version information for pascal so far. I.e. we can wait for versions info support in pascal until requested.

AlexPeshkoff avatar Oct 18 '22 13:10 AlexPeshkoff