Export missing symbols and fix CMakeLists.txt for modules

[mikomikotaishi]

What does this change do?
This commit amends the module file to export some missing symbols, such as those of the name is_*, updates CMakeLists.txt in modules to fix a failure to compile, and renames tomlpp.cppm to tomlplusplus.cppm, so that the file name aligns with the name of the module.

Is it related to an exisiting bug report or feature request?
No, but it is related to a previous commit (#266).

Pre-merge checklist

• I've read CONTRIBUTING.md
• I've rebased my changes against the current HEAD of origin/master (if necessary)
• I've added new test cases to verify my change
• I've regenerated toml.hpp (how-to)
• I've updated any affected documentation
• I've rebuilt and run the tests with at least one of:
  • Clang 8 or higher
  • GCC 8 or higher
  • MSVC 19.20 (Visual Studio 2019) or higher
• I've added my name to the list of contributors in README.md

[mikomikotaishi]

@marzer @N-Dekker I noticed some parts of the CMakeLists.txt were removed (the target_sources() call, unless I'm mistaken), I ended up re-adding it because it wouldn't compile without it, could you remind me if there was a reason it was removed? Making these modifications allows me to use all the symbols perfectly fine though, and so I'm hoping to make sure there isn't something I'm missing on my end.

[mikomikotaishi]

Are there any concerns or questions about my PR before you can have it merged?

[marzer]

Hi there, erm, no concerns for me personally. I'm only keeping an eye on the modules stuff from a distance, so any changes here I think are largely between you and @N-Dekker to figure out 😅 (I don't know what the reasoning was behind the symbol export changes 🤷🏼‍♂️)

[mikomikotaishi]

OK, sounds good. Let's hear what @N-Dekker has to say about the CMake changes then before finalising these changes

[N-Dekker]

Thanks for your effort @mikomikotaishi Unfortunately, I don't have experience with C++ modules, other than by trying out your contribution! And so far, I could not make the tomlplusplus module work on my side, mainly because of MSVC compiler bugs.

For a feature like this, I think it's necessary to have unit tests, running on the CI. (Unless we want to keep it in some sort of experimental status, of course.) However, for me it would already help if it can be made compatible with MSVC, so that I can try it out at home 🤷 @marzer Can you please merge my pull request #271? Then afterwards I can give this PR (#273) a try on MSVC.

[N-Dekker]

@marzer Thanks for merging my pull request #271 Sorry I kept it in "draft" status for too long! Obviously it's just a workaround, to make MSVC happy!

@mikomikotaishi Can you please rebase you pull request branch, so that I can try it out with MSVC?

Do a git checkout of your branch (patch-2), and do:

git fetch upstream master:master
git rebase master
git push --force

Assuming upstream is the name you have for the remote https://github.com/marzer/tomlplusplus.git

[mikomikotaishi]

I merged the changes in, try it now @N-Dekker

[mikomikotaishi]

@N-Dekker Did you have any trouble getting it to work? Let me know if anything went wrong

[N-Dekker]

Did you have any trouble getting it to work?

No, I just didn't have time yet. Is there a specific deadline for this feature, that I can take into account?

[mikomikotaishi]

No, I suppose not, but it would be nice to merge it as soon as possible. I have started using the tomlplusplus module myself and because it is missing those symbols it creates problems which I have to manually resolve.

[N-Dekker]

As there are no unit tests for the module, I'm just locally adjusting an existing test (example), "examples/error_printer.cpp", trying to get it to work with the module: Instead of the #include <toml++/toml.hpp>, I do:

import tomlplusplus;
#define TOML_EXCEPTIONS 1

Unfortunately it still produces link errors with MSVC

>error_printer.obj : error LNK2001: unresolved external symbol "public: static unsigned char const * const toml::v3::impl::utf8_decoder::state_table" (?state_table@utf8_decoder@impl@v3@toml@@2QBEB)
>error_printer.obj : error LNK2001: unresolved external symbol "private: static unsigned __int64 const toml::v3::impl::impl_ex::parser::max_nested_values" (?max_nested_values@parser@impl_ex@impl@v3@toml@@0_KB)
  ...
>error_printer.obj : error LNK2001: unresolved external symbol "public: static class std::basic_string_view<char,struct std::char_traits<char> > const toml::v3::impl::parse_integer_traits<10>::full_prefix" (?full_prefix@?$parse_integer_traits@$09@impl@v3@toml@@2V?$basic_string_view@DU?$char_traits@D@std@@@std@@B)
>B:\tomlplusplus\examples\Debug\error_printer.exe : fatal error LNK1120: 18 unresolved externals

Are you able to locally adjust your "examples/error_printer.cpp" to do import tomlplusplus, and then build and run it?

[mikomikotaishi]

I used it here in this file that just re-exports all the symbols in the library, and it compiles fine on Clang.

Also, can you explain why you added #define TOML_EXCEPTIONS 1? I am not sure what that does, as modules do not export macros.

[N-Dekker]

I used it here in this file that just re-exports all the symbols in the library, and it compiles fine on Clang.

@mikomikotaishi Thank you for trying! But sorry, I feel it's a pity that you do not use the module implementation from toml++ itself. It's your own proposal! It will give other users more trust in this great new toml++ feature, when the original author of the feature uses it directly!

Also, can you explain why you added #define TOML_EXCEPTIONS 1? I am not sure what that does, as modules do not export macros.

Thanks for asking, @mikomikotaishi ! The problem is that the example itself does #if TOML_EXCEPTIONS:

tomlplusplus/examples/error_printer.cpp

Line 164 in 8eb4012

#if TOML_EXCEPTIONS

I haven't yet figures out how to deal with macro's that are part of the interface, when using modules. If I understand correctly, end-users should be able to have such an #if TOML_EXCEPTIONS in their code, right, @marzer ? So then it should be exported somehow 🤷

[marzer]

Well, they don't have to have it, since it's optionally-user configurable, with a default value if the user hasn't overridden it.

But yeah probably whatever that value is, be it defaulted or user-specified, should find it's way into the code that consumes the module.

[mikomikotaishi]

@marzer @N-Dekker I see, however the matter with TOML_EXCEPTIONS deciding whether the implementations will throw an exception would have to be decided when tomlplusplus is compiled, not during the compilation of the translation units that consume the module. Thus it would not be up to those units to do so, but rather the user, who would have to specify that macro at compilation. So adding TOML_EXCEPTIONS inside the importing files probably won't do anything.

It will give other users more trust in this great new toml++ feature, when the original author of the feature uses it directly!

I would argue I am using it, I just have to re-alias those types to match the PascalCase style I am using for object names.

But yeah probably whatever that value is, be it defaulted or user-specified, should find it's way into the code that consumes the module.

Modules cannot export macros, header units (headers that are imported rather than #includeed) can however.

Anyway, could the PR be merged now? The PR here mostly pertains to features that I forgot to add in the first PR and I would just want to have that rectified.

[marzer]

Modules cannot export macros, header units (headers that are imported rather than #includeed) can however.

I mean, sure, but that's somewhat a distinction without a difference for most people given the current state of tooling, and not really what I meant. Header units are bridging measure introduced as part of the broader C++ modules language feature. People consuming the library that way need the macro in the same way people using regular #includes do.

Anyway, could the PR be merged now?

Yeah, sure, it looks fine to me