On Tue, Jun 30, 2020 at 06:50:54PM -0400, y2s1982 . via Gcc wrote:
> 2020-06-30 Tony Sim <y2s1...@gmail.com>
>
> libgomp/ChangeLog:
>
> * libgompd.h : Add gompd_callbacks variable.
No space before :, but generally, you should be exact on what changed.
So
* libgompd.h: Include omp-tools.h.
(gompd_callbacks): New declaration.
> * ompd-lib.c (ompd_finalize): Add new function.
And similarly here describe the other changes too.
> +ompd_callbacks_t* gompd_callbacks;
Formatting is wrong, space should be before * and not afterwards.
And, it should be extern too, you don't want to
have a definition in all TUs that include libgompd.h.
But, I don't really see the need for indirection, why there isn't just
extern ompd_callbacks_t gompd_callbacks;
?
> +
> #endif /* LIBGOMPD_H */
> diff --git a/libgomp/ompd-lib.c b/libgomp/ompd-lib.c
> index f0ae9e85a7e..4513dadc070 100644
> --- a/libgomp/ompd-lib.c
> +++ b/libgomp/ompd-lib.c
> @@ -28,6 +28,7 @@
>
> #include "omp-tools.h"
> #include "libgompd.h"
> +#include <stdlib.h>
And somewhere in this file
ompd_callbacks_t gompd_callbacks;
>
> ompd_rc_t
> ompd_get_api_version (ompd_word_t *version)
> @@ -49,13 +50,25 @@ ompd_initialize (ompd_word_t api_version, const
> ompd_callbacks_t *callbacks)
> {
> static int ompd_initialized = 0;
>
> + if (!callbacks)
> + return ompd_rc_bad_input;
> +
> if (ompd_initialized)
> return ompd_rc_error;
>
> + gompd_callbacks = malloc(sizeof(ompd_callbacks_t));
Formatting: there should be spaces before each ( here, so
malloc (sizeof (ompd_callbacks_t)).
But, as written in OMPD, it really shouldn't use malloc/free itself,
instead it should use the callbacks to allocate and free heap memory.
But if gompd_callbacks doesn't have an indirection, you can just
gompd_callbacks = *callbacks;
and be done with it.
> + *gompd_callbacks = *callbacks;
Formatting: no double space before =, just one space.
> +
> (void) api_version;
> - (void) callbacks;
>
> ompd_initialized = 1;
>
> return ompd_rc_ok;
> }
> +
> +ompd_rc_t
> +ompd_finalize(void)
And space before ( again.
> +{
> + free (gompd_callbacks);
And if gompd_callbacks isn't a pointer, no need to free anything.
Otherwise, you should use a callback to free the memory.
> + return ompd_rc_ok;
> +}
> --
> 2.27.0
>
Jakub
