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

Restructure inline namespace and allow version component to be disabled #3683

Merged
merged 5 commits into from
Aug 10, 2022

Conversation

falbrechtskirchinger
Copy link
Contributor

@falbrechtskirchinger falbrechtskirchinger commented Aug 6, 2022

Restructure the inline namespace, moving the version component to the end and renaming the prefix to json_abi.

Example: nlohmann::json_abi_diag_v3_11_1

The prefix of the tag namespace cannot be json as that would conflict with the type alias nlohmann::json.

The version namespace can be disabled by defining NLOHMANN_JSON_NAMESPACE_NO_VERSION to 1.

@coveralls
Copy link

coveralls commented Aug 6, 2022

Coverage Status

Coverage remained the same at 100.0% when pulling 5385b05 on falbrechtskirchinger:inline-ns-update into a92ccaf on nlohmann:develop.

@gregmarr
Copy link
Contributor

gregmarr commented Aug 6, 2022

What is the benefit of having the version part a separate namespace that may or may not be used instead of just a part of the name that may or may not be there?

@falbrechtskirchinger
Copy link
Contributor Author

It just expresses the hierarchy between the truly (and always) ABI-breaking settings and library versions. I don't really know yet if that's useful, but I don't think it hurts either.

@falbrechtskirchinger falbrechtskirchinger force-pushed the inline-ns-update branch 5 times, most recently from 91fad9d to a2ac34c Compare August 7, 2022 06:31
@falbrechtskirchinger
Copy link
Contributor Author

ci_test_single_header failing is expected until #3679 is merged.

Copy link
Contributor Author

@falbrechtskirchinger falbrechtskirchinger 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
Copy link
Owner

nlohmann commented Aug 7, 2022

Is there a way to get back to the original behavior and just have a nlohmann namespace with basic_json right in it without inline namespaces?

@falbrechtskirchinger
Copy link
Contributor Author

falbrechtskirchinger commented Aug 7, 2022

Yes. Since 3.11.0.
#3657 (comment)
(And the test has been added to this PR.)

@falbrechtskirchinger
Copy link
Contributor Author

falbrechtskirchinger commented Aug 7, 2022

I've added a first, rough version of features/namespace.md. Clearly needs more polish in the future. Looking forward to incorporating any suggestions you may have.

@nlohmann nlohmann mentioned this pull request Aug 7, 2022
nlohmann::nlohmann_json_diag::v3_11_2
```

## Disabling the version namespace
Copy link
Owner

@nlohmann nlohmann Aug 8, 2022

Choose a reason for hiding this comment

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

I had another thought on the documentation.

The current documentation describes what the namespace is and how to manipulate the default behavior. Maybe it should be structured as follows:

  • describe that starting in 3.11.0 we changed the namespace from nlohmann to a more elaborate version
  • describe why this is a benefit - maybe by explicitly listing the incompatibilities that were possible before, but would not have been caught before; maybe also mention JSON_SKIP_LIBRARY_VERSION_CHECK
  • describe how the namespace is structured
  • describe the 3 different styles the namespace can be configured:
    • default - note: this is fine until we have a better means to describe ABI compatibility (right?)
    • no version part - note: this is fine if the library user wants to take the risk of mixing libraries with different versions, but with same ABI tags
    • nlohmann - note: use at your own risk (maybe also show the approach of extern from/to_json result in linker error #3657 (comment))

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.

I already had planned on addressing this, so thanks for the outline.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't quite adopt the outline and made a note to take another look and incorporate more of your ideas.

For now, I think what I have is as good as it's going to get in 2 passes.

@falbrechtskirchinger falbrechtskirchinger changed the title Split inline namespace and allow version namespace to be disabled Restructure inline namespace and allow version component to be disabled Aug 9, 2022
@falbrechtskirchinger
Copy link
Contributor Author

Here's how the plantuml diagram in features/namespace.md renders:

namespace md_uml

Copy link
Contributor Author

@falbrechtskirchinger falbrechtskirchinger left a comment

Choose a reason for hiding this comment

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

Minor doc issue rest looks good to me.

  • ABI test produces namespace names as expected.
  • GDB pretty printer works with the new namespace layout.
  • mkdocs pass style check.
  • Examples outputs regenerated.
  • docset index is complete (minus operator pages already in upstream/develop).
  • Did I forget anything?

docs/mkdocs/docs/api/macros/index.md Outdated Show resolved Hide resolved
types.

There is an exception when forward declarations are used (i.e., when including `json_fwd.hpp`) in which case the linker
may complain about undefined references.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to extend this part with an example but don't have it in me right now. :-)

To do so, define [`NLOHMANN_JSON_NAMESPACE_NO_VERSION`](../api/macros/nlohmann_json_namespace_no_version.md) to `1`.

This applies to version 3.11.2 and above only, versions 3.11.0 and 3.11.1 can apply the technique described in the next
section to emulate the effect of the `NLOHMANN_JSON_NAMESPACE_NO_VERSION` macro.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not provide an example here as this will hopefully be an exceedingly rare use case.

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 added this to the Release 3.11.2 milestone Aug 10, 2022
@nlohmann nlohmann merged commit 0e61ee8 into nlohmann:develop Aug 10, 2022
@falbrechtskirchinger falbrechtskirchinger deleted the inline-ns-update branch August 10, 2022 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants