Skip to content

Allow tomlplusplus to be built by C++20 modules#266

Merged
marzer merged 11 commits intomarzer:masterfrom
mikomikotaishi:master
Mar 31, 2025
Merged

Allow tomlplusplus to be built by C++20 modules#266
marzer merged 11 commits intomarzer:masterfrom
mikomikotaishi:master

Conversation

@mikomikotaishi
Copy link
Copy Markdown
Contributor

@mikomikotaishi mikomikotaishi commented Mar 28, 2025

What does this change do?

This allows tomlplusplus to optionally be built with C++20 modules.

Is it related to an exisiting bug report or feature request?

No, but this may potentially be a suggested feature by users of C++20 and later. (Mentioned by #172, though that is for a header unit rather than a module.)

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
Copy link
Copy Markdown
Contributor Author

Alternatively the module that it exports could be named just "tomlpp" instead of "tomlplusplus", for convenience

Copy link
Copy Markdown
Owner

@marzer marzer left a comment

Choose a reason for hiding this comment

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

Ooooh! Very exciting!

Is it related to an exisiting bug report or feature request?
No, but this may potentially be a suggested feature by users of C++20 and later.

There was an issue related to modules, #172, though the submitter closed it for some reason. (cc @wroyca)

@wroyca
Copy link
Copy Markdown

wroyca commented Mar 28, 2025

Hello!

That was 3 years ago; I don't remember why I closed it. If anything, modules were in a much worse state back then, so it's really not a good reference point. Feel free to treat that old issue report as if it didn't exist. :)

@mikomikotaishi
Copy link
Copy Markdown
Contributor Author

Hi, are there still any changes you are looking for before you accept and merge my pull request?

@marzer
Copy link
Copy Markdown
Owner

marzer commented Mar 29, 2025

@mikomikotaishi

Hi, are there still any changes you are looking for before you accept and merge my pull request?

Two things:

  • Your files are missing trailing line endings. A minor annoyance, but please address it.
  • @N-Dekker asks some good questions that I'm also curious to hear the answer to.

After that I'm happy to see this merged 👍🏼

@mikomikotaishi
Copy link
Copy Markdown
Contributor Author

@mikomikotaishi

Hi, are there still any changes you are looking for before you accept and merge my pull request?

Two things:

* Your files are missing trailing line endings. A minor annoyance, but please address it.

* @N-Dekker asks some good questions that I'm also curious to hear the answer to.

After that I'm happy to see this merged 👍🏼

OK, changes have been made as requested

@N-Dekker
Copy link
Copy Markdown
Contributor

@mikomikotaishi Thanks very much for addressing my questions regarding the CMake variable CXX_STANDARD and toml::get_line! How did you test your new feature? I'm wondering if there could be some kind of test for this feature on the CI as well, not sure if that's feasible 🤷 Ideally I guess all unit tests could optionally be built using the module, instead of the header file. As in:

#if TOML_TEST_MODULE
import toml;
#else
#include <toml++/toml.hpp>
#endif

The tests could then be built either with TOML_TEST_MODULE=0 or with TOML_TEST_MODULE=1, and both should then work equally well, right? I don't know if that's feasible for now. But at least some kind of test of this interesting new feature would certainly be nice. What do you think?

@mikomikotaishi
Copy link
Copy Markdown
Contributor Author

mikomikotaishi commented Mar 29, 2025

@mikomikotaishi Thanks very much for addressing my questions regarding the CMake variable CXX_STANDARD and toml::get_line! How did you test your new feature? I'm wondering if there could be some kind of test for this feature on the CI as well, not sure if that's feasible 🤷 Ideally I guess all unit tests could optionally be built using the module, instead of the header file. As in:

#if TOML_TEST_MODULE

import toml;

#else

#include <toml++/toml.hpp>

#endif


The tests could then be built either with TOML_TEST_MODULE=0 or with TOML_TEST_MODULE=1, and both should then work equally well, right? I don't know if that's feasible for now. But at least some kind of test of this interesting new feature would certainly be nice. What do you think?

To be honest, I haven't written any example tests bundled with this project but I do have a basically identical copy (only difference is I aliased all type names to PascalCase) of all the files in this pull request in one of my own projects, which compiles and uses tomlplusplus fine. Here is my project that uses tomlplusplus, it compiles fine and the code works.

As for bundling some tests with this project, I admit I'm honestly not that great with CMake, I only know enough to get my own projects and this library to compile, but not how to link example test cases to locate libraries correctly for whatever reason. I can try bundling your suggestions with the macros though

@N-Dekker
Copy link
Copy Markdown
Contributor

Thanks @mikomikotaishi Honestly I do have quite some CMake experience, but I haven't yet tried out C++ modules, I have to admit. I'm still stuck at C++17 🤷

When your pull request is merged, would you use it yourself, for your project, replacing your original "openJuice/tomlpp/tomlpp.cppm" file?

Comment on lines +21 to +24
inline namespace literals {
using toml::literals::operator""_toml;
using toml::literals::operator""_tpath;
}
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.

Do you use Visual C++? I just tried it out, using the latest update of Visual Studio 2022 (version 17.13.5). Unfortunately if fails to export the operators from toml::literals, saying:

>S:\tomlplusplus\src\modules\tomlpp.cppm(22,21): error C2872: 'literals': ambiguous symbol
>    S:\tomlplusplus\src\modules\tomlpp.cppm(21,22):
>    could be 'toml::literals'
>    S:\tomlplusplus\include\toml++\impl\parser.hpp(322,19):
>    or       'toml::v3::literals'
>S:\tomlplusplus\src\modules\tomlpp.cppm(22,31): error C2039: '""_toml': is not a member of 'toml::literals'
>    S:\tomlplusplus\src\modules\tomlpp.cppm(21,22):
>    see declaration of 'toml::literals'
>S:\tomlplusplus\src\modules\tomlpp.cppm(22,9): error C2873: '""_toml': symbol cannot be used in a using-declaration
>S:\tomlplusplus\src\modules\tomlpp.cppm(23,21): error C2872: 'literals': ambiguous symbol
>    S:\tomlplusplus\src\modules\tomlpp.cppm(21,22):
>    could be 'toml::literals'
>    S:\tomlplusplus\include\toml++\impl\parser.hpp(322,19):
>    or       'toml::v3::literals'
>S:\tomlplusplus\src\modules\tomlpp.cppm(23,31): error C2039: '""_tpath': is not a member of 'toml::literals'
>    S:\tomlplusplus\src\modules\tomlpp.cppm(21,22):
>    see declaration of 'toml::literals'
>S:\tomlplusplus\src\modules\tomlpp.cppm(23,9): error C2873: '""_tpath': symbol cannot be used in a using-declaration

Do you have a clue how to fix this?

Copy link
Copy Markdown
Contributor Author

@mikomikotaishi mikomikotaishi Mar 30, 2025

Choose a reason for hiding this comment

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

That's very strange. I use Clang and no such error occurs on my end. I can look at it a bit more closely in a bit
For now try commenting those out and trying again? I always thought MSVC was the most well-equipped for modules funnily enough

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.

I don't know whether the "ambiguous symbol" errors are caused by a compiler bug. But it appears that these MSVC errors go away when explicitly using the operators from toml::v3::literals. As follows:

    inline namespace literals {
        using toml::v3::literals::operator""_toml;
        using toml::v3::literals::operator""_tpath;
    }

Hope that helps!

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.

It may be an inline namespace thing I think, I'll try changing that to use the v3 scope and see if it works on Clang as well

@N-Dekker
Copy link
Copy Markdown
Contributor

N-Dekker commented Mar 30, 2025

I just tried your module in simple_parser.cpp. Did a CMake build, having CMAKE_CXX_STANDARD "20", TOMLPLUSPLUS_BUILD_MODULES and BUILD_EXAMPLES. Added import tomlplusplus; as first line of code, and removed #include <toml++/toml.hpp>. Here:

#include <toml++/toml.hpp>
Added the generated "tomlpp.cppm.ifc" file to Visual Studio, simple_parser, C/C++ Additional Module Dependencies.

Unfortunately the compiler says:

>------ Build started: Project: simple_parser, Configuration: Debug x64 ------
>simple_parser.cpp
>S:\tomlplusplus\include\toml++\impl\value.hpp(270,39): error C3878: syntax error: unexpected token ',' following 'simple-type-specifier'
>(compiling source file 'S:/T/tomlplusplus/examples/simple_parser.cpp')
>    S:\tomlplusplus\include\toml++\impl\value.hpp(270,39):
>    missing one of: '(' '{' ?
>    S:\tomlplusplus\include\toml++\impl\value.hpp(270,39):
>    the template instantiation context (the oldest one first) is
>        S:\tomlplusplus\include\toml++\impl\parser.inl(2757,26):
>        while compiling class template member function 'toml::v3::value<int64_t>::value<int64_t,0>(int64_t &&) noexcept(<expr>)'
>S:\tomlplusplus\include\toml++\impl\value.hpp(269,43): error C2057: expected constant expression
>(compiling source file 'S:/T/tomlplusplus/examples/simple_parser.cpp')
>Done building project "simple_parser.vcxproj" -- FAILED.

Not sure if it's an MSVC bug, I'm using the latest VS2022 update (version 17.13.5). Any clue?


PS @mikomikotaishi Could you please check if simple_parser does compile (and run) with Clang, when you let it do import tomlplusplus; instead of #include <toml++/toml.hpp>?

@mikomikotaishi
Copy link
Copy Markdown
Contributor Author

Thanks @mikomikotaishi Honestly I do have quite some CMake experience, but I haven't yet tried out C++ modules, I have to admit. I'm still stuck at C++17 🤷

When your pull request is merged, would you use it yourself, for your project, replacing your original "openJuice/tomlpp/tomlpp.cppm" file?

That file tomlpp.cppm exports all the symbols in PascalCase, which is the convention I am using in that project, so if I were to use it I would have to make a few adjustments to re-exporting them in my own project the way I aliased them there. At the moment I would probably just replace the #include with an import there, because modules can't be re-aliased/renamed

@mikomikotaishi
Copy link
Copy Markdown
Contributor Author

mikomikotaishi commented Mar 30, 2025

PS @mikomikotaishi Could you please check if simple_parser does compile (and run) with Clang, when you let it do import tomlplusplus; instead of #include <toml++/toml.hpp>?

Sure I'll test it and let you know, might take a bit of CMake trickery but given my past experience with it it does seem to work

@mikomikotaishi
Copy link
Copy Markdown
Contributor Author

@N-Dekker It seems to work on Clang for me, maybe it's a weird compiler issue

@mikomikotaishi
Copy link
Copy Markdown
Contributor Author

@marzer Any update on this, is it ready to be merged?

@marzer
Copy link
Copy Markdown
Owner

marzer commented Mar 31, 2025

Nope, looks all good, thanks 😄

@marzer marzer merged commit fea1d90 into marzer:master Mar 31, 2025
3 checks passed
gogoalalmh-dotcom

This comment was marked as spam.

@DmitriiKuchevskii
Copy link
Copy Markdown

Sorry for that, but I have a silly question
How do I use module? The only way I could make it work is

  • Install headers into system dir
  • Copy tomlplusplus.cppm into my project and add it into CXX_MODULES (in cmake file)

Is it somehow possible to import the module w/o adding .cppm file into the project?

@mikomikotaishi
Copy link
Copy Markdown
Contributor Author

Hi,

I am a little confused what you mean. The module is declared in that file, so naturally it must be part of the project to be imported. Could you clarify what it is you would like?

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.

6 participants