-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Fix some C++ warnings #3079
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 some C++ warnings #3079
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -703,7 +703,7 @@ _NODISCARD pair<_CharT*, errc> __d2fixed_buffered_n(_CharT* _First, _CharT* cons | |||||||||
|
|
||||||||||
| _NODISCARD inline to_chars_result __d2exp_buffered_n(char* _First, char* const _Last, const double __d, | ||||||||||
| uint32_t __precision) { | ||||||||||
| char* const _Original_first = _First; | ||||||||||
| const char* const _Original_first = _First; | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
The leading whitespace is unnecessary. Other than that, more |
||||||||||
|
|
||||||||||
| const uint64_t __bits = __double_to_bits(__d); | ||||||||||
|
|
||||||||||
|
|
@@ -1832,11 +1832,11 @@ _NODISCARD inline __floating_decimal_64 __d2d(const uint64_t __ieeeMantissa, con | |||||||||
|
|
||||||||||
| // Step 4: Find the shortest decimal representation in the interval of valid representations. | ||||||||||
| int32_t __removed = 0; | ||||||||||
| uint8_t __lastRemovedDigit = 0; | ||||||||||
| uint64_t _Output; | ||||||||||
| // On average, we remove ~2 digits. | ||||||||||
| if (__vmIsTrailingZeros || __vrIsTrailingZeros) { | ||||||||||
| // General case, which happens rarely (~0.7%). | ||||||||||
| uint8_t __lastRemovedDigit = 0; | ||||||||||
| // General case, which happens rarely (~0.7%). | ||||||||||
|
Comment on lines
+1838
to
+1839
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
More bad whitespace detected. Also, I feel like the comment should be above the variable declaration. However, moving |
||||||||||
| for (;;) { | ||||||||||
| const uint64_t __vpDiv10 = __div10(__vp); | ||||||||||
| const uint64_t __vmDiv10 = __div10(__vm); | ||||||||||
|
|
@@ -2331,8 +2331,7 @@ _NODISCARD pair<_CharT*, errc> __d2s_buffered_n(_CharT* const _First, _CharT* co | |||||||||
| } | ||||||||||
|
|
||||||||||
| __floating_decimal_64 __v; | ||||||||||
| const bool __isSmallInt = __d2d_small_int(__ieeeMantissa, __ieeeExponent, &__v); | ||||||||||
| if (__isSmallInt) { | ||||||||||
| if (const bool __isSmallInt = __d2d_small_int(__ieeeMantissa, __ieeeExponent, &__v)) { | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Better yet, just remove |
||||||||||
| // For small integers in the range [1, 2^53), __v.__mantissa might contain trailing (decimal) zeros. | ||||||||||
| // For scientific notation we need to move these zeros into the exponent. | ||||||||||
| // (This is not needed for fixed-point notation, so it might be beneficial to trim | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1241,7 +1241,7 @@ protected: | |
| _OutIt _Dest, ios_base& _Iosbase, _Elem _Fill, bool _Val) const { // put formatted bool to _Dest | ||
| if (!(_Iosbase.flags() & ios_base::boolalpha)) { | ||
| return do_put(_Dest, _Iosbase, _Fill, static_cast<long>(_Val)); | ||
| } else { // put "false" or "true" | ||
| } else { // put "true" or "false" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is kind of silly, in my opinion. I guess "true or false" rolls off of the tongue better than "false or true," but is something like this really worth the effort? No changes are suggested here. Keep in mind, however, that reviewers have to look over every change you make. (I still feel bad for the reviewers who had to endure the copious amounts of non-conformance and style differences in my own PR.) |
||
| const auto& _Punct_fac = _STD use_facet<numpunct<_Elem>>(_Iosbase.getloc()); | ||
| basic_string<_Elem> _Str; | ||
| if (_Val) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -149,9 +149,9 @@ namespace pmr { | |||||||
| } | ||||||||
|
|
||||||||
| private: | ||||||||
| virtual void* do_allocate(size_t _Bytes, size_t _Align) = 0; | ||||||||
| virtual void do_deallocate(void* _Ptr, size_t _Bytes, size_t _Align) = 0; | ||||||||
| virtual bool do_is_equal(const memory_resource& _That) const noexcept = 0; | ||||||||
| virtual void* do_allocate(size_t _Bytes, size_t _Align) = 0; | ||||||||
| virtual void do_deallocate(void* _Ptr, size_t _Bytes, size_t _Align) = 0; | ||||||||
| _NODISCARD virtual bool do_is_equal(const memory_resource& _That) const noexcept = 0; | ||||||||
| }; | ||||||||
|
|
||||||||
| _NODISCARD inline bool operator==(const memory_resource& _Left, const memory_resource& _Right) noexcept { | ||||||||
|
|
@@ -269,7 +269,8 @@ namespace pmr { | |||||||
| } | ||||||||
|
|
||||||||
| template <class _Uty> | ||||||||
| _CXX17_DEPRECATE_POLYMORPHIC_ALLOCATOR_DESTROY void destroy(_Uty* const _Ptr) noexcept /* strengthened */ { | ||||||||
| _CXX17_DEPRECATE_POLYMORPHIC_ALLOCATOR_DESTROY void destroy(_Uty* const _Ptr) noexcept | ||||||||
| /* strengthened */ { | ||||||||
|
Comment on lines
+272
to
+273
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Why add a newline here? The definition for |
||||||||
| _Destroy_in_place(*_Ptr); | ||||||||
| } | ||||||||
|
|
||||||||
|
|
||||||||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -85,6 +85,7 @@ void _Thrd_yield() { // surrender remainder of timeslice | |||
| SwitchToThread(); | ||||
| } | ||||
|
|
||||
| extern "C" { | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||
| // TRANSITION, ABI: _Thrd_equal() is preserved for binary compatibility | ||||
| _CRTIMP2_PURE int _Thrd_equal(_Thrd_t thr0, _Thrd_t thr1) { // return 1 if thr0 and thr1 identify same thread | ||||
| return thr0._Id == thr1._Id; | ||||
|
|
@@ -97,6 +98,7 @@ _CRTIMP2_PURE _Thrd_t _Thrd_current() { // return _Thrd_t identifying current th | |||
| result._Id = GetCurrentThreadId(); | ||||
| return result; | ||||
| } | ||||
| } | ||||
|
|
||||
| _Thrd_id_t _Thrd_id() { // return unique id for current thread | ||||
| return GetCurrentThreadId(); | ||||
|
|
||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,9 +42,8 @@ _MRTIMP2 void __cdecl _Atexit(void(__cdecl* pf)()) { // add to wrapup list | |
| struct _Init_atexit { // controller for atexit processing | ||
| __CLR_OR_THIS_CALL ~_Init_atexit() noexcept { // process wrapup functions | ||
| while (atcount_cdecl < _Nats) { | ||
| const auto pf = reinterpret_cast<void(__cdecl*)()>( | ||
| DecodePointer(reinterpret_cast<void*>(atfuns_cdecl[atcount_cdecl++]))); | ||
| if (pf) { | ||
| if (const auto pf = reinterpret_cast<void(__cdecl*)()>( | ||
| DecodePointer(reinterpret_cast<void*>(atfuns_cdecl[atcount_cdecl++])))) { | ||
|
Comment on lines
+45
to
+46
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I actually prefer the old code on this better, it will be easier to set breakpoints to debug the old way. The way in this PR makes it much more harder to set breakpoint for assignment and the if separately.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I actually like the changed version better. Setting a breakpoint at an assignment is kind of silly unless it is to walk through the code which follows it (since assigning to a variable is trivial). Using Visual Studio, if you set a breakpoint at the No changes are requested from me here. |
||
| pf(); | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -107,15 +107,15 @@ void __CLRCALL_PURE_OR_CDECL locale::_Locimp::_Locimp_ctor( | |||||
| void __CLRCALL_PURE_OR_CDECL locale::_Locimp::_Locimp_Addfac( | ||||||
| _Locimp* _This, locale::facet* ptrfac, size_t id) { // add a facet to a locale | ||||||
| _BEGIN_LOCK(_LOCK_LOCALE) | ||||||
| const size_t MINCAT = 40; // minimum number of facets in a locale | ||||||
| constexpr size_t MINCAT = 40; // minimum number of facets in a locale | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is fine. |
||||||
|
|
||||||
| if (_This->_Facetcount <= id) { // make facet vector larger | ||||||
| size_t count = id + 1; | ||||||
| if (count < MINCAT) { | ||||||
| count = MINCAT; | ||||||
| } | ||||||
|
|
||||||
| locale::facet** ptrnewvec = | ||||||
| const auto ptrnewvec = | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is also ok.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
This is another case of |
||||||
| static_cast<locale::facet**>(_realloc_crt(_This->_Facetvec, count * sizeof(locale::facet**))); | ||||||
| if (ptrnewvec == nullptr) { // report no memory | ||||||
| _Xbad_alloc(); | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -146,7 +146,7 @@ _MRTIMP2_PURE locale __CLRCALL_PURE_OR_CDECL locale::empty() { // make empty tra | |
| } | ||
|
|
||
| _MRTIMP2_PURE locale::_Locimp* __CLRCALL_PURE_OR_CDECL locale::_Init(bool _Do_incref) { // setup global and "C" locales | ||
| locale::_Locimp* ptr = nullptr; | ||
| locale::_Locimp* ptr; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why the removal of nullptr here?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Presumably, they did that because the (On another note, I really don't like those |
||
|
|
||
| _BEGIN_LOCK(_LOCK_LOCALE) // prevent double initialization | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -52,8 +52,8 @@ static void mul( | |
|
|
||
| void __CLRCALL_PURE_OR_CDECL _MP_Mul( | ||
| _MP_arr w, unsigned long long u0, unsigned long long v0) noexcept { // multiply multi-word value by multi-word value | ||
| constexpr int m = 2; | ||
| constexpr int n = 2; | ||
| constexpr unsigned int m = 2; | ||
| constexpr unsigned int n = 2; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This file I am mostly ok with, except that I have no idea why the removal of |
||
| unsigned long long u[2]; | ||
| unsigned long long v[2]; | ||
| u[0] = u0 & mask; | ||
|
|
@@ -63,16 +63,16 @@ void __CLRCALL_PURE_OR_CDECL _MP_Mul( | |
|
|
||
| // Knuth, vol. 2, p. 268, Algorithm M | ||
| // M1: [Initialize.] | ||
| for (int i = 0; i < m + n + 1; ++i) { | ||
| for (unsigned int i = 0; i < m + n + 1; ++i) { | ||
| w[i] = 0; | ||
| } | ||
|
|
||
| for (int j = 0; j < n; ++j) { // M2: [Zero multiplier?] | ||
| for (unsigned int j = 0; j < n; ++j) { // M2: [Zero multiplier?] | ||
| if (v[j] == 0) { | ||
| w[j + m] = 0; | ||
| } else { // multiply by non-zero value | ||
| unsigned long long k = 0; | ||
| int i; | ||
| unsigned int i; | ||
| // M3: [Initialize i.] | ||
| for (i = 0; i < m; ++i) { // M4: [Multiply and add.] | ||
| w[i + j] = u[i] * v[j] + w[i + j] + k; | ||
|
|
@@ -170,7 +170,6 @@ void __CLRCALL_PURE_OR_CDECL _MP_Rem( | |
| } | ||
| // D5: [Test remainder.] | ||
| if (k != 0) { // D6: [Add back.] | ||
| --qh; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As AraHaan mentioned, this change affects the logic of the code. Could you explain why it was made? Was the removal intentional? |
||
| add(u + j, n + 1, v, n); | ||
| } | ||
| // D7: [Loop on j.] | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,8 +38,9 @@ extern "C" _CRTIMP2 void __cdecl __set_stl_sync_api_mode(__stl_sync_api_modes_en | |
|
|
||
| struct _Mtx_internal_imp_t { // ConcRT mutex | ||
| int type; | ||
| typename std::_Aligned_storage<Concurrency::details::stl_critical_section_max_size, | ||
| Concurrency::details::stl_critical_section_max_alignment>::type cs; | ||
| std::_Aligned_storage_t<Concurrency::details::stl_critical_section_max_size, | ||
| Concurrency::details::stl_critical_section_max_alignment> | ||
| cs; | ||
|
Comment on lines
+41
to
+43
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see nothing wrong with the original here too. |
||
| long thread_id; | ||
| int count; | ||
| Concurrency::details::stl_critical_section_interface* _get_cs() { // get pointer to implementation | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,12 +46,10 @@ namespace Concurrency { | |
|
|
||
| #else // defined(_CRT_APP) || defined(UNDOCKED_WINDOWS_UCRT) | ||
|
|
||
| namespace Concurrency { | ||
| namespace details { | ||
| namespace Concurrency::details { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why the reformat of the namespaces like this? |
||
|
|
||
| _CRTIMP2 void __thiscall _ExceptionHolder::ReportUnhandledError() {} | ||
| _CRTIMP2 void __thiscall _ExceptionHolder::ReportUnhandledError() {} | ||
|
|
||
| } // namespace details | ||
| } // namespace Concurrency | ||
| } // namespace Concurrency::details | ||
|
|
||
| #endif // defined(_CRT_APP) || defined(UNDOCKED_WINDOWS_UCRT) | ||
Uh oh!
There was an error while loading. Please reload this page.