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 declaration for LMIC_DEBUG_PRINTF_FN #41

Merged
merged 2 commits into from
Dec 15, 2017

Conversation

frankleonrose
Copy link

Fix to #39.

@terrillmoore
Copy link
Member

Hi Frank, I agree that we should do something like this.

The (undocumented) design intention was that the definition of LMIC_DEBUG_PRINTF_FN, if used, would come from the file pointed to by LMIC_DEBUG_INCLUDE; and so that file would also provide the prototype. I did not want to require that LMIC_DEBUG_PRINTF_FN be the name of a function, and I didn't want to dictate the prototype.

Other code is already using LMIC_DEBUG_PRINTF_FN with LMIC_DEBUG_INCLUDE, and so is already providing a prototype. I'm a little worried about providing the prototype in this way.

We could reliably detect a "command-line definition" (your use case) by checking a few thing before we include LMIC_DEBUG_INCLUDE. We'd have something like this:

#if LMIC_DEBUG_LEVEL > 0
# if defined(LMIC_DEBUG_PRINTF_FN) && ! defined(LMIC_DEBUG_INCLUDE)
//
// If defined at this point, LMIC_DEBUG_PRINTF_FN came from the build system.
// We don't have  LMIC_DEBUG_INCLUDE, so we need a prototype..
//
void LMIC_DEBUG_PRINTF_FN(const char *fmt, ...);
# endif

# ifdef LMIC_DEBUG_INCLUDE
// ... current code
# endif

# ifndef LMIC_DEBUG_PRINTF
// ... code as at present.

What do you think? I think this would be less intrusive. [Equivalently, and possibly less confusingly, we could do:

# ifdef LMIC_DEBUG_INCLUDE
...
# else if defined(LMIC_DEBUG_PRINTF_FN)
void LMIC_DEBUG_PRITNF_FN(const char *, ...);
# endif

(I now kind of like the second way better.)
--Terry

@frankleonrose
Copy link
Author

How about just wrapping the declaration in #ifndef LMIC_DEBUG_INCLUDE?

#     ifndef LMIC_DEBUG_INCLUDE // If you use LMIC_DEBUG_INCLUDE, put the declaration in there
        void LMIC_DEBUG_PRINTF_FN(const char *f, ...);
#     endif // ndef LMIC_DEBUG_INCLUDE

Your second way would require repetition of the #define LMIC_DEBUG_PRINTF line and doesn't seem more clear.

#if defined(LMIC_DEBUG_INCLUDE)
...stringify...include...
# if !defined(LMIC_DEBUG_PRINTF)
#   ifdef LMIC_DEBUG_PRINTF_FN
#     define LMIC_DEBUG_PRINTF(f, ...) LMIC_DEBUG_PRINTF_FN(f, ## __VA_ARGS__) <-- same
#   else // ndef LMIC_DEBUG_PRINTF_FN
#     define LMIC_DEBUG_PRINTF(f, ...) printf(f, ## __VA_ARGS__)
#   endif // ndef LMIC_DEBUG_PRINTF_FN
# endif
#elif !defined(LMIC_DEBUG_PRINTF) && defined(LMIC_DEBUG_PRINTF_FN)
#     define LMIC_DEBUG_PRINTF(f, ...) LMIC_DEBUG_PRINTF_FN(f, ## __VA_ARGS__) <-- same
        void LMIC_DEBUG_PRINTF_FN(const char *f, ...);
# endif

@terrillmoore
Copy link
Member

How about just wrapping the declaration in #ifndef LMIC_DEBUG_INCLUDE?

Agreed, much better. Will you push a change in that format or shall I?

@frankleonrose
Copy link
Author

Done.

@terrillmoore terrillmoore merged commit 42dfb39 into mcci-catena:master Dec 15, 2017
@terrillmoore terrillmoore deleted the fix-print-function branch December 15, 2017 16:21
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.

2 participants