On 12/02/2014 08:35 PM, Mike Holmes wrote:

    - * ODP default LOG macro.
    + * ODP log function
    + *
    + * Instead of direct prints to stdout/stderr all logging in ODP
    implementation
    + * should be done via this function or its wrappers.
    + * ODP platform MUST provide a default *weak* implementation of
    this function.

MUST -> must

This follows our specification approach to use RFC 2119.


    + * Application MAY override the function if needed by providing a
    strong


MAY -> may

    + * function.
    + *
    + * @param level   Log level


doxygen in or out

    + * @param fmt     printf-style message format


doxygen in or out

    + */


need @return description or if possible @retval if there are specific
cases

Will add.


    +extern int odp_override_log(odp_log_level_e level, const char *fmt,
    ...);


level does not appear to be the best name, these are output streams with
different characteristics rather than a single stream with different levels

No. This is a level. It even has type of odp_log_level_e.
Mapping it to streams is an implementation/application choice.


    +
    +/**
    + * ODP LOG macro.
       */
      #define ODP_LOG(level, fmt, ...) \
      do { \
             switch (level) { \
             case ODP_LOG_ERR: \
    -               fprintf(stderr, "%s:%d:%s():" fmt, __FILE__, \
    +               odp_override_log(level, "%s:%d:%s():" fmt, __FILE__, \
                     __LINE__, __func__, ##__VA_ARGS__); \
                     break; \
             case ODP_LOG_DBG: \
                     if (ODP_DEBUG_PRINT == 1) \
    -                       fprintf(stderr, "%s:%d:%s():" fmt, __FILE__, \
    +                       odp_override_log(level, "%s:%d:%s():" fmt,
    __FILE__, \
                             __LINE__, __func__, ##__VA_ARGS__); \
                     break; \
             case ODP_LOG_PRINT: \
    -               fprintf(stdout, " " fmt, ##__VA_ARGS__); \
    +               odp_override_log(level, " " fmt, ##__VA_ARGS__); \
                     break; \
             case ODP_LOG_ABORT: \
    -               fprintf(stderr, "%s:%d:%s(): " fmt, __FILE__, \
    +               odp_override_log(level, "%s:%d:%s(): " fmt, __FILE__, \
                     __LINE__, __func__, ##__VA_ARGS__); \
                     abort(); \
                     break; \
             case ODP_LOG_UNIMPLEMENTED: \
    -               fprintf(stderr, \
    +               odp_override_log(level, \
                             "%s:%d:The function %s() is not
    implemented\n" \
                             fmt, __FILE__, __LINE__, __func__,
    ##__VA_ARGS__); \
                     break; \
             default: \
    -               fprintf(stderr, "Unknown LOG level"); \
    +               odp_override_log(level, "Unknown LOG level"); \
                     break;\
             } \
      } while (0)

      /**
    - * Printing macro, which prints output when the application
    - * calls one of the ODP APIs specifically for dumping internal data.
    + * Log print message when the application calls one of the ODP APIs
    + * specifically for dumping internal data.
       */
      #define ODP_PRINT(fmt, ...) \
                     ODP_LOG(ODP_LOG_PRINT, fmt, ##__VA_ARGS__)

      /**
    - * Debug printing macro, which prints output when DEBUG flag is set.
    + * Log debug message if DEBUG flag is set.


The flag is ODP_DEBUG_PRINT

I didn't change the original flag name. Will update it.


       */
      #define ODP_DBG(fmt, ...) \
                     ODP_LOG(ODP_LOG_DBG, fmt, ##__VA_ARGS__)

      /**
    - * Print output to stderr (file, line and function).
    + * Log error message.


We should say that this is not maskable

None of them is maskable except DEBUG level. Do you want to add
an explicit note to each of them?


       */
      #define ODP_ERR(fmt, ...) \
                     ODP_LOG(ODP_LOG_ERR, fmt, ##__VA_ARGS__)

      /**
    - * Print output to stderr (file, line and function),
    - * then abort.
    + * Log abort message and then stop execution (by default call abort()).
    + * This function should not return.
       */
      #define ODP_ABORT(fmt, ...) \
                     ODP_LOG(ODP_LOG_ABORT, fmt, ##__VA_ARGS__)
    diff --git a/platform/linux-generic/odp_weak.c
    b/platform/linux-generic/odp_weak.c
    new file mode 100644
    index 0000000..fccbc3f
    --- /dev/null
    +++ b/platform/linux-generic/odp_weak.c
    @@ -0,0 +1,23 @@
    +/* Copyright (c) 2014, Linaro Limited
    + * All rights reserved.
    + *
    + * SPDX-License-Identifier:     BSD-3-Clause
    + */
    +
    +#include <odp_internal.h>
    +#include <odp_debug.h>
    +#include <odp_debug_internal.h>
    +#include <odp_hints.h>
    +
    +ODP_WEAK_SYMBOL int odp_override_log(odp_log_level_e level ODP_UNUSED,
    +                                    const char *fmt, ...)


Why is this ODP_UNUSED, is it needed ?

Because level is unused in this default definition.

To use it a reimplementation of this function by an application would
effectively have to have another switch statement.
Generating that need for two switches implies we should be exporting
overrides for each log type and not
prematurely factorising our code.

If we do that we just remove our switch statement and put the
implementation under the weak function that each macro directly calls.

Have you looked at the next patch (#2)? I especially haven't done those
changes in a first patch to keep it minimal.

_______________________________________________
lng-odp mailing list
[email protected]
http://lists.linaro.org/mailman/listinfo/lng-odp

Reply via email to