-
-
Notifications
You must be signed in to change notification settings - Fork 182
fix MSVC C++20 module build error C2872, C2039, C2873 on 'literals' #267
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
Conversation
src/modules/tomlpp.cppm
Outdated
using TOML_NAMESPACE::literals::operator""_tpath; | ||
} | ||
|
||
using toml::array; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marzer Do you think the other using toml::<identifier>
s within the export namespace toml
clause should also be replaced with using TOML_NAMESPACE::<identifier>
?`
It does compile, when doing:
using TOML_NAMESPACE::array;
using TOML_NAMESPACE::date;
using TOML_NAMESPACE::date_time;
...
using TOML_NAMESPACE::parse_file;
Instead of the current:
tomlplusplus/src/modules/tomlpp.cppm
Lines 26 to 66 in fea1d90
using toml::array; | |
using toml::date; | |
using toml::date_time; | |
using toml::inserter; | |
using toml::json_formatter; | |
using toml::key; | |
using toml::node; | |
using toml::node_view; | |
using toml::parse_error; | |
using toml::parse_result; | |
using toml::path; | |
using toml::path_component; | |
using toml::source_position; | |
using toml::source_region; | |
using toml::table; | |
using toml::time; | |
using toml::time_offset; | |
using toml::toml_formatter; | |
using toml::value; | |
using toml::yaml_formatter; | |
using toml::format_flags; | |
using toml::node_type; | |
using toml::path_component_type; | |
using toml::value_flags; | |
using toml::array_iterator; | |
using toml::const_array_iterator; | |
using toml::const_table_iterator; | |
using toml::default_formatter; | |
using toml::inserted_type_of; | |
using toml::optional; | |
using toml::source_index; | |
using toml::source_path_ptr; | |
using toml::table_iterator; | |
using toml::at_path; | |
using toml::get_line; | |
using toml::operator""_toml; | |
using toml::operator""_tpath; | |
using toml::operator<<; | |
using toml::parse; | |
using toml::parse_file; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, hmmn, yeah. I completely forgot about all of that machinery.
In theory you should be able to simplify it by replacing
export namespace toml
{
//...
}
with
export TOML_NAMESPACE_START
{
// ...
}
because TOML_NAMESPACE_START
is shorthand for namespace toml { inline namespace <internal abi namespace>
. And/or individually qualifying them, yes.
I'll be honest in saying that I know very little about modules, so I am not too sure of the right approach here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @marzer So far I haven't been able to get export TOML_NAMESPACE_START
compiling, so for now I'm proposing to just replace toml
with TOML_NAMESPACE
in those using-declarations. That will at least make MSVC slightly happier.
(Doing export TOML_NAMESPACE_START
might be a nicer follow-up, of course)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the record, when I locally did export TOML_NAMESPACE_START
... TOML_NAMESPACE_END;
in "src/modules/tomlpp.cppm", the module still compiled well, but "examples/simple_parser" encountered an internal compiler error! (I locally adjusted "simple_parser.cpp" by doing import tomlplusplus;
instead of #include <toml++/toml.hpp>
.) It produced the following output:
------ Build started: Project: simple_parser, Configuration: Debug x64 ------
simple_parser.cpp
S:\tomlplusplus\examples\simple_parser.cpp(30,20): error C1001: Internal compiler error.
S:\tomlplusplus\examples\simple_parser.cpp(30,20): error C1001: (compiler file 'D:\a\_work\1\s\src\vctools\Compiler\CxxFE\sl\p1\cxx\function-signature.cpp', line 212)
S:\tomlplusplus\examples\simple_parser.cpp(30,20): error C1001: To work around this problem, try simplifying or changing the program near the locations listed above.
S:\tomlplusplus\examples\simple_parser.cpp(30,20): error C1001: If possible please provide a repro here: https://developercommunity.visualstudio.com
S:\tomlplusplus\examples\simple_parser.cpp(30,20): error C1001: Please choose the Technical Support command on the Visual C++
S:\tomlplusplus\examples\simple_parser.cpp(30,20): error C1001: Help menu, or open the Technical Support help file for more information
Done building project "simple_parser.vcxproj" -- FAILED.
using toml::literals::operator""_toml; | ||
using toml::literals::operator""_tpath; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marzer @mikomikotaishi I noticed that operator""_toml
and operator""_tpath
are exported twice.
Here, inside toml::literals
:
tomlplusplus/src/modules/tomlpp.cppm
Lines 21 to 24 in fea1d90
inline namespace literals { | |
using toml::literals::operator""_toml; | |
using toml::literals::operator""_tpath; | |
} |
And here, at the root of the
toml
namespace: tomlplusplus/src/modules/tomlpp.cppm
Lines 62 to 63 in fea1d90
using toml::operator""_toml; | |
using toml::operator""_tpath; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I thought it would be necessary to re-export the contents of literals
, and on my end it compiles fine - but if it is only causing more harm than benefit, perhaps it should be removed entirely? I am not entirely sure what the literals
namespace is for myself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mikomikotaishi Well, it just isn't clear to me why operator""_toml
and operator""_tpath
are exported twice (once in toml::literals
and once in toml
), but if you have a use case that only works when they are exported twice, can you please share it?
Anyway, this pull request already appears to fix those MSVC compile errors on literals
, so technically it seems to work, exporting those operators twice.
Replaced `toml` with `TOML_NAMESPACE` in the using-declarations in the exported namespace. Fixed the errors from Visual Studio 2022 version 17.13.5 Compiler Version 19.43.34809 that appeared when building "tomlplusplus_modules", saying: src\modules\tomlpp.cppm(22,21): error C2872: 'literals': ambiguous symbol src\modules\tomlpp.cppm(21,22): could be 'toml::literals' include\toml++\impl\parser.hpp(322,19): or 'toml::v3::literals' src\modules\tomlpp.cppm(22,31): error C2039: '""_toml': is not a member of 'toml::literals' src\modules\tomlpp.cppm(21,22): see declaration of 'toml::literals' src\modules\tomlpp.cppm(22,9): error C2873: '""_toml': symbol cannot be used in a using-declaration
f7df288
to
d97bc6f
Compare
@N-Dekker sorry, just coming back to this now. It looks complete, but was there anything still outstanding? |
@marzer No problem! The pull request itself is complete, I think. Unfortunately, there were still some other compile errors, when I last tried the module with MSVC. But these errors were unrelated. 🤷 I hope I was just missing an MSVC patch, when I tried! |
What does this change do?
Replaced
toml
withTOML_NAMESPACE
in the using-declarations in the exported namespace.Fixed the errors from Visual Studio 2022 version 17.13.5 Compiler Version 19.43.34809 that appeared when building "tomlplusplus_modules", saying:
Is it related to an existing bug report or feature request?
Pre-merge checklist
origin/master
(if necessary)