Skip to content
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

fix int -> uint warning #2611

Merged
merged 5 commits into from
Dec 1, 2021
Merged

Conversation

Acretock
Copy link
Contributor

in my project i tried to mute warnings from fmt, but this one persist, should be harmless.

@vitaut
Copy link
Contributor

vitaut commented Nov 24, 2021

Could you post the complete warning message and platform details?

@Acretock
Copy link
Contributor Author

Acretock commented Nov 25, 2021

Could you post the complete warning message and platform details?

warnings, build with Android Studio on Windows10

warning: implicit conversion loses integer precision: 'int' to 'mode_t' (aka 'unsigned short') [-Wimplicit-int-conversion]
warning: implicit conversion changes signedness: 'int' to 'mode_t' (aka 'unsigned int') [-Wsign-conversion]

src/os.cc Outdated
@@ -218,7 +218,7 @@ int buffered_file::fileno() const {

#if FMT_USE_FCNTL
file::file(cstring_view path, int oflag) {
int mode = S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH;
unsigned int mode = S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH;
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't work on Windows where mode is int. The solution would be to use mode_t here and define it near the constants in https://github.com/fmtlib/fmt/blob/master/src/os.cc#L32-L43

Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't warning disappear if constexpr is used, so that the compiler will have to substitute the value and not being afraid of narrowing. Then it could be constexpr auto I guess.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think constexpr is relevant here and auto will just result in int. Note that the warning is not on this line.

src/os.cc Outdated Show resolved Hide resolved
src/os.cc Outdated Show resolved Hide resolved
src/os.cc Outdated
Comment on lines 47 to 49
# ifndef mode_t
# define mode_t int
# endif
Copy link
Contributor

Choose a reason for hiding this comment

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

For win32:

#ifndef __MINGW32__
using mode_t = int;
#endif

src/os.cc Outdated Show resolved Hide resolved
@phprus
Copy link
Contributor

phprus commented Nov 29, 2021

@vitaut , @Acretock

Mingw always defines the _mode_t type. And in addition, the mode_t type is defined (if the NO_OLDNAMES macro is not defined).

See:
https://github.com/mirror/mingw-w64/blob/19ecabd807e3429ecf88263107911958b63d4e76/mingw-w64-headers/crt/sys/types.h#L72-L79

Maybe define the type like this:

#ifdef _WIN32
#  ifdef __MINGW32__
using fmt_mode_t = _mode_t;
#  else
using fmt_mode_t = int;
#  endif
#else
using fmt_mode_t = mode_t;
#endif

And then use the fmt_mode_t type?

src/os.cc Outdated
# ifdef __MINGW32__
# define _SH_DENYNO 0x40
# else
using mode_t = int;
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's put mode_t in the fmt::detail namespace and in the file constructor add using namespace fmt::detail. Then it will pick up our fmt::detail::mode_t if available and the global one otherwise. Also no need to check for __MINGW32__.

src/os.cc Outdated Show resolved Hide resolved
src/os.cc Show resolved Hide resolved
@vitaut vitaut merged commit 9b1807a into fmtlib:master Dec 1, 2021
@vitaut
Copy link
Contributor

vitaut commented Dec 1, 2021

Thank you

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.

None yet

4 participants