Program fails to build if windows.h is included after toml++.

[ImTouchk]

Environment

toml++ version and/or commit hash: dca6945

Compiler: MSVC 19.28.29915

C++ standard mode (e.g. 17, 20, 'latest'): latest/20

Target arch (e.g. x64): x64

Library configuration overrides: -

Relevant compilation flags: -

Describe the bug

Including windows.h after toml++/toml.h (but not the other way around) causes the build to fail:
Error C2733: MultiByteToWideChar/WideCharToMultiByte: you cannot overload a function with extern "C" linkage.

Steps to reproduce (or a small repro code sample)

#include <toml++/toml.h>
#include <windows.h>

int main()
{
	return 0;
}

Library is added as a cmake subproject and built at the same time.

Additional information

The issue happens in this header: toml_default_formatter.hpp, at line 205. I managed to get around this error by placing the declarations in the root namespace and guard them with a macro:

#ifndef _WINDOWS_
extern "C"
{
    int __stdcall WideCharToMultiByte(unsigned int CodePage, unsigned long dwFlags, const wchar_t* lpWideCharStr, int cchWideChar,
                                        const char* lpMultiByteStr, int cbMultiByte, const char* lpDefaultChar, int* lpUsedDefaultChar);
    int __stdcall MultiByteToWideChar(unsigned int CodePage, unsigned long dwFlags, const char* lpMultiByteStr, int cbMultiByte,
                                    const wchar_t* lpWideCharStr, int cchWideChar);
}
#endif

Though I'm not sure how it'd be fixed without removing the winapi namespace.

[marzer]

Ah, good catch! I think your solution is a good one; them being in a namespace isn't overly critical.

[marzer]

Fixed in master. Thanks @ImTouchk!

[levicki]

@marzer I don't understand how what you did here fixes the problem?

For me the build for my project now fails with error C2375 while it worked flawlessly before this change:

1>------ Build started: Project: TOMLConfig, Configuration: Debug Win32 ------
1>TOMLConfig.cpp
1>C:\Program Files (x86)\Windows Kits\10\Include\10.0.19041.0\um\stringapiset.h(126,1): error C2375: 'MultiByteToWideChar': redefinition; different linkage
1>C:\Work\Projects\PS\CameraService\3rdParty\toml++\toml.hpp(12113): message : see declaration of 'MultiByteToWideChar'
1>C:\Program Files (x86)\Windows Kits\10\Include\10.0.19041.0\um\stringapiset.h(141,1): error C2375: 'WideCharToMultiByte': redefinition; different linkage
1>C:\Work\Projects\PS\CameraService\3rdParty\toml++\toml.hpp(12103): message : see declaration of 'WideCharToMultiByte'
1>Done building project "TOMLConfig.vcxproj" -- FAILED.

Your definition for those functions is also incorrect as it omits both SAL annotations and WINBASEAPI qualifier (which expands to __declspec(dllimport)), resulting in the above error.

I would appreciate if you do not copy function definitions from platform header files, but instead include appropriate platform header file if needed, and rely on their existing guards to avoid function redefinitions.

If you are concerned about namespace pollution, there are a lot of macros that can be defined prior to including windows.h that will reduce it:

/*  If defined, the following flags inhibit definition
 *     of the indicated items.
 *
 *  NOGDICAPMASKS     - CC_*, LC_*, PC_*, CP_*, TC_*, RC_
 *  NOVIRTUALKEYCODES - VK_*
 *  NOWINMESSAGES     - WM_*, EM_*, LB_*, CB_*
 *  NOWINSTYLES       - WS_*, CS_*, ES_*, LBS_*, SBS_*, CBS_*
 *  NOSYSMETRICS      - SM_*
 *  NOMENUS           - MF_*
 *  NOICONS           - IDI_*
 *  NOKEYSTATES       - MK_*
 *  NOSYSCOMMANDS     - SC_*
 *  NORASTEROPS       - Binary and Tertiary raster ops
 *  NOSHOWWINDOW      - SW_*
 *  OEMRESOURCE       - OEM Resource values
 *  NOATOM            - Atom Manager routines
 *  NOCLIPBOARD       - Clipboard routines
 *  NOCOLOR           - Screen colors
 *  NOCTLMGR          - Control and Dialog routines
 *  NODRAWTEXT        - DrawText() and DT_*
 *  NOGDI             - All GDI defines and routines
 *  NOKERNEL          - All KERNEL defines and routines
 *  NOUSER            - All USER defines and routines
 *  NONLS             - All NLS defines and routines
 *  NOMB              - MB_* and MessageBox()
 *  NOMEMMGR          - GMEM_*, LMEM_*, GHND, LHND, associated routines
 *  NOMETAFILE        - typedef METAFILEPICT
 *  NOMINMAX          - Macros min(a,b) and max(a,b)
 *  NOMSG             - typedef MSG and associated routines
 *  NOOPENFILE        - OpenFile(), OemToAnsi, AnsiToOem, and OF_*
 *  NOSCROLL          - SB_* and scrolling routines
 *  NOSERVICE         - All Service Controller routines, SERVICE_ equates, etc.
 *  NOSOUND           - Sound driver routines
 *  NOTEXTMETRIC      - typedef TEXTMETRIC and associated routines
 *  NOWH              - SetWindowsHook and WH_*
 *  NOWINOFFSETS      - GWL_*, GCL_*, associated routines
 *  NOCOMM            - COMM driver routines
 *  NOKANJI           - Kanji support stuff.
 *  NOHELP            - Help engine interface.
 *  NOPROFILER        - Profiler interface.
 *  NODEFERWINDOWPOS  - DeferWindowPos routines
 *  NOMCX             - Modem Configuration Extensions
 */

However, I wouldn't advise doing that unless there is an actual conflict (such as with min and max macros colliding with std::min and std::max).

To be perfectly clear, I do agree that the order of include directives shouldn't break anything and that the reported issue should be fixed -- I just disagree with this particular "fix".

[marzer]

@levicki

I don't understand how what you did here fixes the problem?

It doesn't fix your problem. It fixes the problem identified in the issue as reported by @ImTouchk. The thing you're experiencing is a different (but related) issue.

Your definition for those functions is also incorrect as it omits both SAL annotations and WINBASEAPI qualifier (which expands to __declspec(dllimport)), resulting in the above error.

Not including SAL annotations has no impact on it's correctness as far as C++ is concerned. They're preprocessor-only hints for tooling (and for the poor humans forced to read them). You're right that it should have WINBASEAPI specifier, though. That is the actual problem here. Interesting that this never came up as an issue for me, even though I use the library on Windows too. Perhaps compiler differences are to blame? I'd need to dig into it to figure out why. That's very odd.

I would appreciate if you do not copy function definitions from platform header files but instead include appropriate platform header file and rely on their existing guards to avoid function redefinitions or if you simply state in documentation and code examples that windows.h should be included before toml++.hpp on Windows platform.

I'd rather not, if I can avoid it. If the library must always be paired with windows.h on Windows, then the library should just include Windows.h itself, as it did originally. The idea behind the change was to avoid the enormous size of Windows.h; I've seen that in a 'blank-slate' project simply including Windows.h can cause compilation speed slowdowns of 500ms - 1s per translation unit even on a very modern PC, which is not ideal. If there's a way to avoid that for the 2 functions I need from Windows.h then I'd like to make use of it. Given that the vast majority of Windows.h is a very very stable API, in most cases it's generally safe to do this sort of thing (as long as it's actually correct, and mine clearly is not!).

If you are concerned about namespace pollution, there are a lot of macros that can be defined prior to including windows.h that will reduce it

Those are useless for a library; if I use them then either my library is the first one to include Windows.h and anyone including it after me would be getting the minimal version of Windows.h (likely to step on what APIs they might need themselves), or my library isn't the first to include Windows.h and they accomplish nothing.

[levicki]

Not including SAL annotations has no impact on it's correctness as far as C++ is concerned.

If I am not mistaken SAL annotations are there to enable compiler to verify correctness of the code calling the function at compile time? If you are sure your code doesn't need them that's good for you.

However, introducing SAL-less definitions that replace windows.h original function definitions will prevent correctness verification in every other code file which also calls those functions where your header file is included. Are you sure that's a good idea?

You're right that it should have WINBASEAPI specifier, though. That is the actual problem here. Interesting that this never came up as an issue for me, even though I use the library on Windows too. Perhaps compiler differences are to blame? I'd need to dig into it to figure out why. That's very odd.

I disagree with what you think the actual problem is -- actual problem is not including windows.h when you need it in your code, and the requirements for using those functions are very clear:

The idea behind the change was to avoid the enormous size of Windows.h; I've seen that in a 'blank-slate' project simply including Windows.h can cause compilation speed slowdowns of 500ms - 1s per translation unit even on a very modern PC, which is not ideal.

Is that with or without using precompiled headers?

Regardless, most projects on Windows platform will have to include windows.h anyway, so the slowdown associated with it cannot really be completely avoided. I think this change was a pointless "optimization" -- you can already avoid recompilation of stable header files by using precompiled header create and use compiler options (/Yc and /Yu) which work very well.

If you are really not comfortable with including windows.h in toml.hpp despite the clear requirements, then why not simply do this:

#ifndef _WINDOWS_
#error Please include windows.h before toml.hpp, preferrably in your project's precompiled header file (stdafx.h or pch.h).
#endif

Given that the vast majority of Windows.h is a very very stable API, in most cases it's generally safe to do this sort of thing (as long as it's actually correct, and mine clearly is not!).

On the contrarry, given that it is a stable API it belongs in the project's stdafx.h or pch.h file. It's not your problem that inexperienced Windows developers don't know that, and IMO you shouldn't pander to them.

Those are useless for a library; ...

I agree, that's why I said I wouldn't recommend that approach.

[marzer]

However, introducing SAL-less definitions that replace windows.h original function definitions will prevent correctness verification in every other code file which also calls those functions where your header file is included. Are you sure that's a good idea?

No it won't. Regular attributes, annotations etc are cumulative. A forward declaration can omit them, and have them added later by a full definition or separate forward declaration. In my experience this applies to SAL annotations, too. My code uses those two functions correctly so I don't benefit from the annotations, but me doing this won't impact SAL for anyone who is including Windows.h for their own use since the SAL annotations will still be applied correctly.

If you are really not comfortable with including windows.h in toml.hpp despite the clear requirements, then why not simply do this [...]

Because that would impose an ordering requirement on #include directives which is almost always a bad idea. I originally had one such requirement with <fstream> but enough people failed to read the documenation and report bugs about it that I realized that I simply can't rely on it being documented. People don't read, unfortunately.

I disagree with what you think the actual problem is -- actual problem is not including windows.h when you need it in your code, and the requirements for using those functions are very clear

Good for you, but it's my library so it's up to me to decide the nature of the issue. I've already outlined my reasoning here and simply do not agree with you about the rest of it. I'll fix the issue by adding the missing API declaration. I'll add an escape-hatch option via a #define so you can have the old behaviour if you want it, but it'll no longer be default.

[marzer]

@levicki your issue should now be fixed in ba75446. Additionally you can use the (undocumented) switch TOML_INCLUDE_WINDOWS_H to override; set it to 1 if you want to explicitly include windows.h, or 0 if you want to use the (now fixed) explicit forward declarations. Both works locally for me but evidently your mileage may vary. Let me know if encounter any issues.

[levicki]

You complain that people don't read documentation, and then you cater to those same people by removing the need to read via hacky workarounds such as this one. I find that really disappointing because it is teaching them the wrong thing.

The proper solution for Windows platform is #error directive that instructs them to include windows.h in a precompiled header which is always included first in any compilation unit so it will naturally come before toml.hpp, and which dramatically reduces compile time as a bonus.

That would teach them to use the platform toolset properly, while this is teaching them that it's OK to copy & paste stuff from random header files (as if copying random bits of code from Stack Overflow wasn't bad enough).

In this particular case it might be benign because API is (mostly) stable, but people extrapolate weirdest things and next thing you know they will copy declarations from STL or Boost or God knows what other 3rd party library where API isn't that stable and when someone else on a team later updates said library all hell will break loose. Your fix is not technically wrong, but IMO it is setting a bad example.

Good for you, but it's my library so it's up to me to decide the nature of the issue.

Now I feel bad for putting you on a defensive.

I apologize for even bringing this up, obviously I was mistaken in my belief that you value my input so I will refrain from reporting any further issues from now on.

[marzer]

@levicki I value input, but your way of communicating it has come to be oddly entitled somehow, as though you're my customer and I owe you something, and it is very demotivating. Your initial communications with me were great but more recently it feels like you've come to act as though you have some ownership of this project, and be more pushy/insistent accordingly. If you can drop that nonsense I'll be more engaged, otherwise please do move on.

[levicki]

@marzer I apologize if I am coming through like that, I can be rather passionate when discussing the future of something I use and like (and I do like your library a lot so let me use this opportunity to thank you again for writing it and allowing us to use it).

However, even if I contributed code instead of just ideas (and opinions like in this case), I would have never considered myself as having any special rights to your work, not even customer relationship, much less ownership. If that's what you are getting from my feedback then either I am not communicating clearly enough, or there must be some sort of language barrier given how English is not my primary language, or perhaps it is just difference in our temperaments.