Skip to content

fix and improve module - #3386

Merged
vitaut merged 1 commit into
fmtlib:masterfrom
DanielaE:master
Apr 18, 2023
Merged

vitaut merged 1 commit into
fmtlib:masterfrom
DanielaE:master

Conversation

@DanielaE

Copy link
Copy Markdown
Contributor
  • export public documented API
  • don't export namespace detail
  • add std.h into module
  • add missing namespace qualification in xchar.h
  • fix call to detail::get_iterator in xchar.h
  • fix ambiguous overload of detail::isfinite in chrono.h

Comment thread include/fmt/chrono.h

@vitaut vitaut left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the PR.

Comment thread include/fmt/core.h Outdated
#endif

FMT_BEGIN_NAMESPACE
FMT_BEGIN_EXPORT

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The symbols below are undocumented and shouldn't be exported.

Comment thread include/fmt/core.h
Comment thread include/fmt/core.h Outdated
}
};
} // namespace detail
FMT_BEGIN_EXPORT

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These are also undocumented APIs that we don't need to export yet.

Comment thread include/fmt/std.h Outdated
}

} // namespace detail
FMT_BEGIN_EXPORT

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Here and a few other places: no need for a block, it's enough to mark the single definition here with FMT_MODULE_EXPORT.

* export public documented API
* don't export `namespace detail`
* add `std.h` into module
* add missing namespace qualification in `xchar.h`
* fix call to `detail::get_iterator` in `xchar.h`
* fix ambiguous overload of `detail::isfinite` in `chrono.h`
Comment thread include/fmt/args.h
}
};
} // namespace detail
FMT_BEGIN_EXPORT

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We only have one class template here so I suggest applying FMT_MODULE_EXPORT to it.

Comment thread include/fmt/core.h
Comment on lines +783 to +785
FMT_MODULE_EXPORT template <typename Context> class basic_format_arg;
FMT_MODULE_EXPORT template <typename Context> class basic_format_args;
FMT_MODULE_EXPORT template <typename Context> class dynamic_format_arg_store;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These are just forward declarations. Do we still need to export them if the main templates are exported?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Names must be declared export when introduced into a namespace [module.interface]/6. The compiler will then collect all semantic properties that pertain to that entity that is referenced by the name. This collection continues throughout the whole module purview up to the end of the TU or the start of the private module fragment, whichever comes first.

In this case, the (forward) declarations introduce the names. So they need to be marked as exported at this very place in the source text.

This reminds me to remove the export declarations from the definitions of the primary templates 🤦‍♂️

@vitaut vitaut mentioned this pull request Apr 17, 2023
@vitaut vitaut changed the title fix and improve module: fix and improve module Apr 18, 2023
@vitaut
vitaut merged commit 0489c19 into fmtlib:master Apr 18, 2023
@vitaut

vitaut commented Apr 18, 2023

Copy link
Copy Markdown
Contributor

Merged, 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.

2 participants