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

Allow overriding THROW/CATCH/TRY macros with no-exceptions #938 #940

Merged
merged 1 commit into from
Jan 29, 2018

Conversation

kaidokert
Copy link
Contributor

No description provided.

@nlohmann
Copy link
Owner

Just a question: is it an issue that the following lines are at the end of the amalgamated JSON header:

#undef JSON_CATCH
#undef JSON_THROW
#undef JSON_TRY

@kaidokert
Copy link
Contributor Author

Hasn't been an issue for me at least, don't need the macros anywhere else, it's better to remove them

@nlohmann
Copy link
Owner

I think it is good practice to undefine the macros you define yourself. So removing the undefs could be an issue.

@kaidokert
Copy link
Contributor Author

Understand .. then i guess the cleanest way to deal with it would be to do this entire dance:

 #if !defined(JSON_THROW)
    #define JSON_THROW(exception) std::abort()
 #else
   #define USER_JSON_THROW 1
#endif
.... // at the end
#if !defined(USER_JSON_THROW)
#undef JSON_THROW
#else
#undef USER_JSON_THROW
#endif

and repeat that for all 3 ?

@nlohmann
Copy link
Owner

I think so. Better call it JSON_THROW_USER_PROVIDED rather than USER_JSON_THROW to have a consistent prefix.

@kaidokert
Copy link
Contributor Author

Updated - i think it need indentation now too, unless its too much commit noise

@nlohmann
Copy link
Owner

If you ran make amalgamate (and had astyle installed), then indentation is already fixed. If not, then this may be an issue in a Travis job.

#define JSON_TRY if(true)
#define JSON_CATCH(exception) if(false)
#if !defined(JSON_THROW)
#define JSON_THROW(exception) std::abort()
Copy link
Owner

Choose a reason for hiding this comment

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

I just realized the code can be simplified:

#if !defined(JSON_THROW)
    #define JSON_THROW(exception) std::abort()
    #define JSON_THROW_DEFAULT
#endif

and later

#if defined(JSON_THROW_DEFAULT)
    #undef JSON_THROW
    #undef JSON_THROW_DEFAULT
#endif

What do you think?

Copy link
Contributor Author

@kaidokert kaidokert Jan 28, 2018

Choose a reason for hiding this comment

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

Yes, this is better, less noise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But then it requires the _DEFAULT defines for exceptions-on version as well, ends up adding a bit of clutter

Copy link
Owner

Choose a reason for hiding this comment

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

Right... But why not allowing to define JSON_THROW etc. while using exceptions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current version in repo does #undef for both exceptions-on and exceptions-off cases. If i rewrite with JSON_THROW_DEFAULT then to keep the same behaviour it would become ..

#if (defined(__cpp_exceptions) ...
    #define JSON_THROW(exception) throw exception
    #define JSON_THROW_DEFAULT //  1 extra line here
   ...
#else
   #if !defined(JSON_THROW)
    #define JSON_THROW(exception) std::abort()
    #define JSON_THROW_DEFAULT
  #endif
  ...
#endif

and later

#if defined(JSON_THROW_DEFAULT)
    #undef JSON_THROW
    #undef JSON_THROW_DEFAULT
#endif

If that is cleaner, I'll rewrite

Copy link
Owner

Choose a reason for hiding this comment

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

Another idea:

// allow to disable exceptions
#if (defined(__cpp_exceptions) || defined(__EXCEPTIONS) || defined(_CPPUNWIND)) && !defined(JSON_NOEXCEPTION)
    #define JSON_THROW(exception) throw exception
    #define JSON_TRY try
    #define JSON_CATCH(exception) catch(exception)
#else
    #define JSON_THROW(exception) std::abort()
    #define JSON_TRY if(true)
    #define JSON_CATCH(exception) if(false)
#endif

// override exception macros
#if define(JSON_THROW_USER)
    #define JSON_THROW JSON_THROW_USER
#endif
#if define(JSON_TRY_USER)
    #define JSON_TRY JSON_TRY_USER
#endif
#if define(JSON_CATCH_USER)
    #define JSON_CATCH JSON_CATCH_USER
#endif

That is, the user can define JSON_THROW_USER to override the behavior of JSON_THROW. No need to differentiate the exception/no exception case, and no need to remember whether we need to undef.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is probably the most hygienic , although for maximum warning compile it should be

#if define(JSON_THROW_USER)
    #undef JSON_THROW
    #define JSON_THROW JSON_THROW_USER
#endif

to avoid macro redefinition warnings

Copy link
Owner

Choose a reason for hiding this comment

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

Right.

@coveralls
Copy link

coveralls commented Jan 28, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling d0c9e5f on kaidokert:develop into b3bd3b7 on nlohmann:develop.

Copy link
Owner

@nlohmann nlohmann 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 to me.

@nlohmann nlohmann self-assigned this Jan 29, 2018
@nlohmann nlohmann added this to the Release 3.1.0 milestone Jan 29, 2018
@nlohmann nlohmann merged commit ae23513 into nlohmann:develop Jan 29, 2018
@nlohmann
Copy link
Owner

Thanks!

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