Instruct msvc to report the true value in __cplusplus
#2353
Conversation
DanielaE
force-pushed
the
feature/__cplusplus-in-module-test
branch
from
f6cd205
to
f73b909
Compare
|
In addition, raise the level of conformance to the C++ standard to the maximum possible to find even more problems. |
include/fmt/core.h
Outdated
| @@ -238,6 +238,7 @@ | |||
|
|
|||
| #ifndef FMT_MODULE_EXPORT | |||
| # define FMT_MODULE_EXPORT | |||
| # define FMT_MODULE_EXPORT_CONSTANT static | |||
There was a problem hiding this comment.
It seems that the name FMT_MODULE_EXPORT_STATIC would be more appropriate.
There was a problem hiding this comment.
Well, this depends on the point of view:
- constants exported from module: external linkage required
- constants in header of a traditional lib: internal linkage recommended
These macros have 'module export' in their name, therefore the choice how to annotate constants in the API surface.
There was a problem hiding this comment.
constants exported from module: external linkage required
What error do you get without external linkage? Would replacing static with inline solve the problem?
There was a problem hiding this comment.
Nothing with internal or no linkage can be exported from any sort of module.
static -> inline at namespace scope is fine because this changes linkage from internal (with possibly multiple instances in the resulting program) to external (with just one instance in the program).
There was a problem hiding this comment.
Then I suggest using inline instead of making this module-dependent. inline seems more appropriate anyway.
There was a problem hiding this comment.
Yeah, but this introduces the problem that inline variables weren't a thing before C++17.
I suggest to just go with 'naked' constexpr variables like f.e. invalid_arg_index in core.h:2651. This is the current state of the updated PR.
src/fmt.cc
Outdated
| @@ -2,6 +2,9 @@ module; | |||
| #ifndef __cpp_modules | |||
| # error Module not supported. | |||
| #endif | |||
| #ifdef _MSC_VER | |||
| # pragma warning(disable : 4702) | |||
There was a problem hiding this comment.
As mentioned in the commit message: it's about silencing warnings about 'unreachable code' bubbling up from the module when affected templates become instantiated in user code.
There was a problem hiding this comment.
Could you post the full warnings here? There might be better ways to suppress than using pragmas.
There was a problem hiding this comment.
From the compile log:
core.h(2668): warning C4702: unreachable code
core.h(2668): warning C4702: unreachable code
core.h(2668): warning C4702: unreachable code
core.h(2668): warning C4702: unreachable code
core.h(2657): warning C4702: unreachable code
core.h(2657): warning C4702: unreachable code
core.h(2668): warning C4702: unreachable code
core.h(2668): warning C4702: unreachable code
core.h(2668): warning C4702: unreachable code
core.h(2657): warning C4702: unreachable code
core.h(2657): warning C4702: unreachable code
I've checked: these pieces of unreachable code can be blessed with putting them into the else prongs of the respective if constexpr ... statements and the warnings vanish.
There was a problem hiding this comment.
I can do that even though it might look messy in the second instance.
There was a problem hiding this comment.
The code would look like
constexpr int invalid_arg_index = -1;
#if FMT_USE_NONTYPE_TEMPLATE_PARAMETERS
template <int N, typename T, typename... Args, typename Char>
constexpr auto get_arg_index_by_name(basic_string_view<Char> name) -> int {
if constexpr (detail::is_statically_named_arg<T>()) {
if (name == T::name) return N;
}
if constexpr (sizeof...(Args) > 0) {
return get_arg_index_by_name<N + 1, Args...>(name);
} else { // **** the following 2 lines are moved into the additional else branch ****
(void)name; // Workaround an MSVC bug about "unused" parameter.
return invalid_arg_index;
}
// **** lines were formerly here ****
}
#endif
template <typename... Args, typename Char>
FMT_CONSTEXPR auto get_arg_index_by_name(basic_string_view<Char> name) -> int {
#if FMT_USE_NONTYPE_TEMPLATE_PARAMETERS
if constexpr (sizeof...(Args) > 0) {
return get_arg_index_by_name<0, Args...>(name);
} else { // **** additional else branch with verbatim copy ***
(void)name;
return invalid_arg_index;
}
#else // **** formerly #endif ****
(void)name;
return invalid_arg_index;
#endif // *** formerly non-existent ****
}
The warning suppression can then be removed again and the code compiles without warnings at /W4 (i.e. STL warning level)
There was a problem hiding this comment.
It doesn't look too bad, let's change get_arg_index_by_name then.
…full_ C++ conformance * do this in _some_ tests to improve test coverage and catch possible problems due to that * fix invalid export of `static constexpr` constant * fix msvc warnings about unreachable code in high warning levels
DanielaE
force-pushed
the
feature/__cplusplus-in-module-test
branch
from
f73b909
to
b1b12ae
Compare
| } | ||
| #endif | ||
|
|
||
| template <typename... Args, typename Char> | ||
| FMT_CONSTEXPR auto get_arg_index_by_name(basic_string_view<Char> name) -> int { | ||
| #if FMT_USE_NONTYPE_TEMPLATE_PARAMETERS | ||
| if constexpr (sizeof...(Args) > 0) | ||
| if constexpr (sizeof...(Args) > 0) { |
There was a problem hiding this comment.
Looks like there will be an unbalanced { if FMT_USE_NONTYPE_TEMPLATE_PARAMETERS is defined.
There was a problem hiding this comment.
No. There are fully balanced if-else branches. Otherwise the code wouldn't compile on my system, like at all. I've checked with FMT_USE_NONTYPE_TEMPLATE_PARAMETERS overridden to both 0and 1, and with 1 being auto-detected.
There was a problem hiding this comment.
My bad, it does look correct.
There was a problem hiding this comment.
@DanielaE: While you on it, please be aware of this:
https://developercommunity.visualstudio.com/t/In-MSVC-16101-Intellisense-and-compil/1449427
There was a problem hiding this comment.
It's not uncommon for EDG (the intellisense compiler) be out of sync with the actual compiler during the preview phase. As long as msvc reports correctly (and it does), we're fine.
|
Merged, thanks. |
Do this in some tests to improve test coverage and catch possible problems due to that.