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

Added to C API: adios2_version_struct. adios2_version(adios2_version_… #3633

Closed
wants to merge 2 commits into from

Conversation

pnorbert
Copy link
Contributor

…struct*) and adios2_version_str() functions.
Fixes #3630

@pnorbert
Copy link
Contributor Author

@vicentebolea: On Linux I get this output mismatch since ADIOS2_VERSION_STR contains the tweak automatically generated by cmake. Can we have another string without it, or generate tweak on all platforms?

$ ./bin/hello_bpWriter_c
ADIOS version as struct 2.9.0
ADIOS version as string 2.9.0.12

Copy link
Collaborator

@vicentebolea vicentebolea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I proposed a change to remove the dynamic alloc and reduce LoC. I do not feel too strong since this is a utility method, let me know :)

Comment on lines +187 to +188
int patch;
} adios2_version_struct;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
int patch;
} adios2_version_struct;
int patch;
char str[sizeof(ADIOS2_VERSION_STR)];
} adios2_version_struct;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That will not work. This struct has a size that is not known in advance, and it is impossible to call it in a dynamic library. Almost every new version of ADIOS2 will have a different size for this struct, leading to ABI incompatibilities.

Why not use a const char* str field that points to a constant string that does not need to be deallocated?

Comment on lines +20 to +26
char *adios2_version_str(void)
{
char *str = (char *)malloc(sizeof(ADIOS2_VERSION_STR));
snprintf(str, sizeof(ADIOS2_VERSION_STR), ADIOS2_VERSION_STR);
return str;
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
char *adios2_version_str(void)
{
char *str = (char *)malloc(sizeof(ADIOS2_VERSION_STR));
snprintf(str, sizeof(ADIOS2_VERSION_STR), ADIOS2_VERSION_STR);
return str;
}

Comment on lines +31 to +32
s->patch = ADIOS2_VERSION_PATCH;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
s->patch = ADIOS2_VERSION_PATCH;
}
s->patch = ADIOS2_VERSION_PATCH;
strncpy(s->str, ADIOS2_VERSION_STR, sizeof(ADIOS2_VERSION_STR));
}

Comment on lines +43 to +45
char *vstr = adios2_version_str();
printf("ADIOS version as string %s\n", vstr);
free(vstr);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
char *vstr = adios2_version_str();
printf("ADIOS version as string %s\n", vstr);
free(vstr);
printf("ADIOS version as string %s\n", s->str);

Comment on lines +24 to +29
/**
* Return library version as string in major.minor.patch format
* String is allocated in the library, must be freed by caller.
*/
char *adios2_version_str(void);

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/**
* Return library version as string in major.minor.patch format
* String is allocated in the library, must be freed by caller.
*/
char *adios2_version_str(void);

@pnorbert pnorbert removed the request for review from williamfgc May 26, 2023 11:33
Comment on lines +187 to +188
int patch;
} adios2_version_struct;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That will not work. This struct has a size that is not known in advance, and it is impossible to call it in a dynamic library. Almost every new version of ADIOS2 will have a different size for this struct, leading to ABI incompatibilities.

Why not use a const char* str field that points to a constant string that does not need to be deallocated?

@@ -180,6 +180,13 @@ typedef struct
adios2_blockinfo *BlocksInfo;
} adios2_varinfo;

typedef struct
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is easier to call functions that don't expect or return a struct. I would prefer

void adios2_version(int *major, int *minor, int *patch);
const char *adios2_version_str(void);

which are overall much simpler to use.

@@ -17,6 +17,20 @@
extern "C" {
#endif

char *adios2_version_str(void)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option would be to use global variables, as in

const int adios2_version_major;
const int adios2_version_minor;
const int adios_version_patch;
const char *const adios2_version_str;

@pnorbert
Copy link
Contributor Author

Remove this pull request for a new one.

@pnorbert pnorbert closed this Jun 25, 2023
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

Successfully merging this pull request may close these issues.

3 participants