You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2023/01/20 09:48:06 UTC
[GitHub] [nuttx] yamt opened a new pull request, #8201: Make 64-bit time_t unsigned
yamt opened a new pull request, #8201:
URL: https://github.com/apache/nuttx/pull/8201
## Summary
* 32-bit time_t should be unsigned because otherwise it wraps too soon. (in 2038)
* 64-bit time_t should be unsigned because it should be consistent within NuttX.
* While signed time_t seems more popular among other OSes, the consisitency within NuttX outweighs, IMO.
## Impact
## Testing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on a diff in pull request #8201: Make 64-bit time_t unsigned
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #8201:
URL: https://github.com/apache/nuttx/pull/8201#discussion_r1094814178
##########
libs/libxx/libcxx.defs:
##########
@@ -71,6 +71,23 @@ libcxx/src/locale.cpp_CXXFLAGS += -Wno-shadow
libcxx/src/filesystem/directory_iterator.cpp_CXXFLAGS += -Wno-shadow
libcxx/src/filesystem/operations.cpp_CXXFLAGS += -Wno-shadow
+# The following warning was observed with icicle:knsh config.
+# Looking at the code in question, it seems harmless to ignore.
+#
+# Note: For some reasons, GCC -Wall enables a different set of warnings
+# for C and C++.
+#
+# References:
+# https://gcc.gnu.org/bugzilla/show_bug.cgi?id=10604
+# https://github.com/gcc-mirror/gcc/blob/e54375d85d4aa5889869c2672158083b2106b623/gcc/c-family/c.opt#L1285-L1287
+#
+# CXX: libcxx/src/condition_variable.cpp
+# libcxx/src/condition_variable.cpp: In member function 'void std::__1::condition_variable::__do_timed_wait(std::__1::unique_lock<std::__1::mutex>&, std::__1::chrono::time_point<std::__1::chrono::system_clock, std::__1::chrono::duration<long long int, std::__1::ratio<1, 1000000000> > >)':
+# libcxx/src/condition_variable.cpp:64:19: error: comparison of integer expressions of different signedness: 'std::__1::chrono::duration<long long int>::rep' {aka 'long long int'} and 'std::__1::__libcpp_numeric_limits<long unsigned int, true>::type' {aka 'long unsigned int'} [-Werror=sign-compare]
+# if (s.count() < ts_sec_max)
+# ~~~~~~~~~~^~~~~~~~~~~~
+libcxx/src/condition_variable.cpp_CXXFLAGS += -Wno-sign-compare
Review Comment:
look like the patch can't pass CI, @yamt ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] yamt commented on pull request #8201: Make 64-bit time_t unsigned
Posted by "yamt (via GitHub)" <gi...@apache.org>.
yamt commented on PR #8201:
URL: https://github.com/apache/nuttx/pull/8201#issuecomment-1400566025
> @yamt ~I think it is dangerous to disable that warning because it could hidden other really issues in C++ code, maybe you can find a way to disable on temporarily to compile that file~ My fault, it is already only for that file. So, why did you keep this PR as draft?
just because i forgot to make it ready.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on a diff in pull request #8201: Make 64-bit time_t unsigned
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #8201:
URL: https://github.com/apache/nuttx/pull/8201#discussion_r1084288842
##########
libs/libxx/libcxx.defs:
##########
@@ -71,6 +71,23 @@ libcxx/src/locale.cpp_CXXFLAGS += -Wno-shadow
libcxx/src/filesystem/directory_iterator.cpp_CXXFLAGS += -Wno-shadow
libcxx/src/filesystem/operations.cpp_CXXFLAGS += -Wno-shadow
+# The following warning was observed with icicle:knsh config.
+# Looking at the code in question, it seems harmless to ignore.
+#
+# Note: For some reasons, GCC -Wall enables a different set of warnings
+# for C and C++.
+#
+# References:
+# https://gcc.gnu.org/bugzilla/show_bug.cgi?id=10604
+# https://github.com/gcc-mirror/gcc/blob/e54375d85d4aa5889869c2672158083b2106b623/gcc/c-family/c.opt#L1285-L1287
+#
+# CXX: libcxx/src/condition_variable.cpp
+# libcxx/src/condition_variable.cpp: In member function 'void std::__1::condition_variable::__do_timed_wait(std::__1::unique_lock<std::__1::mutex>&, std::__1::chrono::time_point<std::__1::chrono::system_clock, std::__1::chrono::duration<long long int, std::__1::ratio<1, 1000000000> > >)':
+# libcxx/src/condition_variable.cpp:64:19: error: comparison of integer expressions of different signedness: 'std::__1::chrono::duration<long long int>::rep' {aka 'long long int'} and 'std::__1::__libcpp_numeric_limits<long unsigned int, true>::type' {aka 'long unsigned int'} [-Werror=sign-compare]
+# if (s.count() < ts_sec_max)
+# ~~~~~~~~~~^~~~~~~~~~~~
+libcxx/src/condition_variable.cpp_CXXFLAGS += -Wno-sign-compare
Review Comment:
libcxx is just one place which generate this warning. Since all major OS typedef time_t to 64bit signed integer, more and more code will generate the same warning, do you want to fix all of them?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] yamt commented on a diff in pull request #8201: Make 64-bit time_t unsigned
Posted by "yamt (via GitHub)" <gi...@apache.org>.
yamt commented on code in PR #8201:
URL: https://github.com/apache/nuttx/pull/8201#discussion_r1093984494
##########
libs/libxx/libcxx.defs:
##########
@@ -71,6 +71,23 @@ libcxx/src/locale.cpp_CXXFLAGS += -Wno-shadow
libcxx/src/filesystem/directory_iterator.cpp_CXXFLAGS += -Wno-shadow
libcxx/src/filesystem/operations.cpp_CXXFLAGS += -Wno-shadow
+# The following warning was observed with icicle:knsh config.
+# Looking at the code in question, it seems harmless to ignore.
+#
+# Note: For some reasons, GCC -Wall enables a different set of warnings
+# for C and C++.
+#
+# References:
+# https://gcc.gnu.org/bugzilla/show_bug.cgi?id=10604
+# https://github.com/gcc-mirror/gcc/blob/e54375d85d4aa5889869c2672158083b2106b623/gcc/c-family/c.opt#L1285-L1287
+#
+# CXX: libcxx/src/condition_variable.cpp
+# libcxx/src/condition_variable.cpp: In member function 'void std::__1::condition_variable::__do_timed_wait(std::__1::unique_lock<std::__1::mutex>&, std::__1::chrono::time_point<std::__1::chrono::system_clock, std::__1::chrono::duration<long long int, std::__1::ratio<1, 1000000000> > >)':
+# libcxx/src/condition_variable.cpp:64:19: error: comparison of integer expressions of different signedness: 'std::__1::chrono::duration<long long int>::rep' {aka 'long long int'} and 'std::__1::__libcpp_numeric_limits<long unsigned int, true>::type' {aka 'long unsigned int'} [-Werror=sign-compare]
+# if (s.count() < ts_sec_max)
+# ~~~~~~~~~~^~~~~~~~~~~~
+libcxx/src/condition_variable.cpp_CXXFLAGS += -Wno-sign-compare
Review Comment:
> libcxx is just one place which generate this warning. Since all major OS typedef time_t to 64bit signed integer, more and more code will generate the same warning, do you want to fix all of them?
of course i don't.
but we need to deal with these warnings for 32-bit time_t anyway.
> BTW, it's very ugly to add -Wno-xxx in a common Make.defs(not all compiler support -Wno-sign-compare), so please modify the code to remove the warning and submit the change to libcxx community.
i will do so sooner or later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make 64-bit time_t unsigned [nuttx]
Posted by "mu578 (via GitHub)" <gi...@apache.org>.
mu578 commented on PR #8201:
URL: https://github.com/apache/nuttx/pull/8201#issuecomment-1872680316
Posix time_t is signed as a consensus. Originally it triggered some controversy and debates over whether it should be signed or unsigned, the problem is not new.
Why does this consensus exist? Because you cannot represent times prior to the Unix epoch, and Posix requires it to be an integral type that's it.
Now, it has an impact on the sign comparison implementation, but it also has an impact on interpolating with other Unix-like sub-systems and/or third-party libraries.
The truth is, there is a consensus and most people are adhering to it. The issue revolves around informal practice. I think that the original change was not a random mistake, but rather an attempt to follow the consensus found in other popular projects.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on a diff in pull request #8201: Make 64-bit time_t unsigned
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #8201:
URL: https://github.com/apache/nuttx/pull/8201#discussion_r1094307347
##########
libs/libxx/libcxx.defs:
##########
@@ -71,6 +71,23 @@ libcxx/src/locale.cpp_CXXFLAGS += -Wno-shadow
libcxx/src/filesystem/directory_iterator.cpp_CXXFLAGS += -Wno-shadow
libcxx/src/filesystem/operations.cpp_CXXFLAGS += -Wno-shadow
+# The following warning was observed with icicle:knsh config.
+# Looking at the code in question, it seems harmless to ignore.
+#
+# Note: For some reasons, GCC -Wall enables a different set of warnings
+# for C and C++.
+#
+# References:
+# https://gcc.gnu.org/bugzilla/show_bug.cgi?id=10604
+# https://github.com/gcc-mirror/gcc/blob/e54375d85d4aa5889869c2672158083b2106b623/gcc/c-family/c.opt#L1285-L1287
+#
+# CXX: libcxx/src/condition_variable.cpp
+# libcxx/src/condition_variable.cpp: In member function 'void std::__1::condition_variable::__do_timed_wait(std::__1::unique_lock<std::__1::mutex>&, std::__1::chrono::time_point<std::__1::chrono::system_clock, std::__1::chrono::duration<long long int, std::__1::ratio<1, 1000000000> > >)':
+# libcxx/src/condition_variable.cpp:64:19: error: comparison of integer expressions of different signedness: 'std::__1::chrono::duration<long long int>::rep' {aka 'long long int'} and 'std::__1::__libcpp_numeric_limits<long unsigned int, true>::type' {aka 'long unsigned int'} [-Werror=sign-compare]
+# if (s.count() < ts_sec_max)
+# ~~~~~~~~~~^~~~~~~~~~~~
+libcxx/src/condition_variable.cpp_CXXFLAGS += -Wno-sign-compare
Review Comment:
Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] acassis commented on pull request #8201: Make 64-bit time_t unsigned
Posted by "acassis (via GitHub)" <gi...@apache.org>.
acassis commented on PR #8201:
URL: https://github.com/apache/nuttx/pull/8201#issuecomment-1400534524
@yamt I think it is dangerous to disable that warning because it could hidden other really issues in C++ code, maybe you can find a way to disable on temporarily to compile that file
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] yamt commented on a diff in pull request #8201: Make 64-bit time_t unsigned
Posted by "yamt (via GitHub)" <gi...@apache.org>.
yamt commented on code in PR #8201:
URL: https://github.com/apache/nuttx/pull/8201#discussion_r1094170744
##########
libs/libxx/libcxx.defs:
##########
@@ -71,6 +71,23 @@ libcxx/src/locale.cpp_CXXFLAGS += -Wno-shadow
libcxx/src/filesystem/directory_iterator.cpp_CXXFLAGS += -Wno-shadow
libcxx/src/filesystem/operations.cpp_CXXFLAGS += -Wno-shadow
+# The following warning was observed with icicle:knsh config.
+# Looking at the code in question, it seems harmless to ignore.
+#
+# Note: For some reasons, GCC -Wall enables a different set of warnings
+# for C and C++.
+#
+# References:
+# https://gcc.gnu.org/bugzilla/show_bug.cgi?id=10604
+# https://github.com/gcc-mirror/gcc/blob/e54375d85d4aa5889869c2672158083b2106b623/gcc/c-family/c.opt#L1285-L1287
+#
+# CXX: libcxx/src/condition_variable.cpp
+# libcxx/src/condition_variable.cpp: In member function 'void std::__1::condition_variable::__do_timed_wait(std::__1::unique_lock<std::__1::mutex>&, std::__1::chrono::time_point<std::__1::chrono::system_clock, std::__1::chrono::duration<long long int, std::__1::ratio<1, 1000000000> > >)':
+# libcxx/src/condition_variable.cpp:64:19: error: comparison of integer expressions of different signedness: 'std::__1::chrono::duration<long long int>::rep' {aka 'long long int'} and 'std::__1::__libcpp_numeric_limits<long unsigned int, true>::type' {aka 'long unsigned int'} [-Werror=sign-compare]
+# if (s.count() < ts_sec_max)
+# ~~~~~~~~~~^~~~~~~~~~~~
+libcxx/src/condition_variable.cpp_CXXFLAGS += -Wno-sign-compare
Review Comment:
> > BTW, it's very ugly to add -Wno-xxx in a common Make.defs(not all compiler support -Wno-sign-compare), so please modify the code to remove the warning and submit the change to libcxx community.
>
> i will do so sooner or later.
https://reviews.llvm.org/D143140
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] yamt commented on pull request #8201: Make 64-bit time_t unsigned
Posted by "yamt (via GitHub)" <gi...@apache.org>.
yamt commented on PR #8201:
URL: https://github.com/apache/nuttx/pull/8201#issuecomment-1445891189
the last push was a conflict resolution
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on a diff in pull request #8201: Make 64-bit time_t unsigned
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #8201:
URL: https://github.com/apache/nuttx/pull/8201#discussion_r1084288842
##########
libs/libxx/libcxx.defs:
##########
@@ -71,6 +71,23 @@ libcxx/src/locale.cpp_CXXFLAGS += -Wno-shadow
libcxx/src/filesystem/directory_iterator.cpp_CXXFLAGS += -Wno-shadow
libcxx/src/filesystem/operations.cpp_CXXFLAGS += -Wno-shadow
+# The following warning was observed with icicle:knsh config.
+# Looking at the code in question, it seems harmless to ignore.
+#
+# Note: For some reasons, GCC -Wall enables a different set of warnings
+# for C and C++.
+#
+# References:
+# https://gcc.gnu.org/bugzilla/show_bug.cgi?id=10604
+# https://github.com/gcc-mirror/gcc/blob/e54375d85d4aa5889869c2672158083b2106b623/gcc/c-family/c.opt#L1285-L1287
+#
+# CXX: libcxx/src/condition_variable.cpp
+# libcxx/src/condition_variable.cpp: In member function 'void std::__1::condition_variable::__do_timed_wait(std::__1::unique_lock<std::__1::mutex>&, std::__1::chrono::time_point<std::__1::chrono::system_clock, std::__1::chrono::duration<long long int, std::__1::ratio<1, 1000000000> > >)':
+# libcxx/src/condition_variable.cpp:64:19: error: comparison of integer expressions of different signedness: 'std::__1::chrono::duration<long long int>::rep' {aka 'long long int'} and 'std::__1::__libcpp_numeric_limits<long unsigned int, true>::type' {aka 'long unsigned int'} [-Werror=sign-compare]
+# if (s.count() < ts_sec_max)
+# ~~~~~~~~~~^~~~~~~~~~~~
+libcxx/src/condition_variable.cpp_CXXFLAGS += -Wno-sign-compare
Review Comment:
libcxx is just one place which generate this warning. Since all major OS typedef time_t to 64bit signed integer, more and more code will generate the same warning, do you want to fix all of them?
BTW, it's very ugly to add -Wno-xxx in a common Make.defs(not all compiler support -Wno-sign-compare), so please modify the code to remove the warning and submit the change to libcxx community.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on a diff in pull request #8201: Make 64-bit time_t unsigned
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #8201:
URL: https://github.com/apache/nuttx/pull/8201#discussion_r1103759996
##########
libs/libxx/libcxx.defs:
##########
@@ -71,6 +71,23 @@ libcxx/src/locale.cpp_CXXFLAGS += -Wno-shadow
libcxx/src/filesystem/directory_iterator.cpp_CXXFLAGS += -Wno-shadow
libcxx/src/filesystem/operations.cpp_CXXFLAGS += -Wno-shadow
+# The following warning was observed with icicle:knsh config.
+# Looking at the code in question, it seems harmless to ignore.
+#
+# Note: For some reasons, GCC -Wall enables a different set of warnings
+# for C and C++.
+#
+# References:
+# https://gcc.gnu.org/bugzilla/show_bug.cgi?id=10604
+# https://github.com/gcc-mirror/gcc/blob/e54375d85d4aa5889869c2672158083b2106b623/gcc/c-family/c.opt#L1285-L1287
+#
+# CXX: libcxx/src/condition_variable.cpp
+# libcxx/src/condition_variable.cpp: In member function 'void std::__1::condition_variable::__do_timed_wait(std::__1::unique_lock<std::__1::mutex>&, std::__1::chrono::time_point<std::__1::chrono::system_clock, std::__1::chrono::duration<long long int, std::__1::ratio<1, 1000000000> > >)':
+# libcxx/src/condition_variable.cpp:64:19: error: comparison of integer expressions of different signedness: 'std::__1::chrono::duration<long long int>::rep' {aka 'long long int'} and 'std::__1::__libcpp_numeric_limits<long unsigned int, true>::type' {aka 'long unsigned int'} [-Werror=sign-compare]
+# if (s.count() < ts_sec_max)
+# ~~~~~~~~~~^~~~~~~~~~~~
+libcxx/src/condition_variable.cpp_CXXFLAGS += -Wno-sign-compare
Review Comment:
@yamt could you update the patch as suggest in https://reviews.llvm.org/D143140?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 merged pull request #8201: Make 64-bit time_t unsigned
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 merged PR #8201:
URL: https://github.com/apache/nuttx/pull/8201
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org