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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Fix some C++ warnings #3079

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions stl/inc/xcharconv_ryu.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
AZero13 marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const char* const _Original_first = _First;
const char* const _Original_first = _First;

The leading whitespace is unnecessary. Other than that, more const is never a bad thing!


const uint64_t __bits = __double_to_bits(__d);

Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
uint8_t __lastRemovedDigit = 0;
// General case, which happens rarely (~0.7%).
// General case, which happens rarely (~0.7%).
uint8_t __lastRemovedDigit = 0;

More bad whitespace detected. Also, I feel like the comment should be above the variable declaration. However, moving __lastRemovedDigit to its most necessary scope is a good change.

for (;;) {
const uint64_t __vpDiv10 = __div10(__vp);
const uint64_t __vmDiv10 = __div10(__vm);
Expand Down Expand Up @@ -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)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (const bool __isSmallInt = __d2d_small_int(__ieeeMantissa, __ieeeExponent, &__v)) {
if (__d2d_small_int(__ieeeMantissa, __ieeeExponent, &__v)) {

Better yet, just remove __isSmallInt altogether if that is its only purpose. (To be fair, I will do things like const bool isSomethingTrue = [. . .]; in my own code if the defining expression is really long or not very self-explanatory, but the comment beneath the if-statement backs up what is going on here and when this case is hit.)

// 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
Expand Down
2 changes: 1 addition & 1 deletion stl/inc/xlocnum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

The 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) {
Expand Down
9 changes: 5 additions & 4 deletions stl/inc/xpolymorphic_allocator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
_CXX17_DEPRECATE_POLYMORPHIC_ALLOCATOR_DESTROY void destroy(_Uty* const _Ptr) noexcept
/* strengthened */ {
_CXX17_DEPRECATE_POLYMORPHIC_ALLOCATOR_DESTROY void destroy(_Uty* const _Ptr) noexcept /* strengthened */ {

Why add a newline here? The definition for select_on_container_copy_construction() immediately beneath this also fits on one line with its /* strengthened */ comment, and that one takes even more characters than this one.

_Destroy_in_place(*_Ptr);
}

Expand Down
2 changes: 1 addition & 1 deletion stl/src/_tolower.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ _CRTIMP2_PURE int __CLRCALL_PURE_OR_CDECL _Tolower(int c, const _Ctypevec* ploc)

// convert wide char to lowercase
size = __crtLCMapStringA(locale_name, LCMAP_LOWERCASE, reinterpret_cast<const char*>(inbuffer), size,
reinterpret_cast<char*>(outbuffer), 3, codepage, TRUE);
reinterpret_cast<char*>(outbuffer), 3, static_cast<int>(codepage), TRUE);
AZero13 marked this conversation as resolved.
Show resolved Hide resolved

if (size == 0) {
return c;
Expand Down
2 changes: 1 addition & 1 deletion stl/src/_toupper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ _CRTIMP2_PURE int __CLRCALL_PURE_OR_CDECL _Toupper(int c, const _Ctypevec* ploc)

// convert wide char to uppercase
size = __crtLCMapStringA(locale_name, LCMAP_UPPERCASE, reinterpret_cast<const char*>(inbuffer), size,
reinterpret_cast<char*>(outbuffer), 3, codepage, TRUE);
reinterpret_cast<char*>(outbuffer), 3, static_cast<int>(codepage), TRUE);
AZero13 marked this conversation as resolved.
Show resolved Hide resolved

if (size == 0) {
return c;
Expand Down
5 changes: 3 additions & 2 deletions stl/src/cond.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,9 @@
#include "primitives.hpp"

struct _Cnd_internal_imp_t { // condition variable implementation for ConcRT
typename std::_Aligned_storage<Concurrency::details::stl_condition_variable_max_size,
Concurrency::details::stl_condition_variable_max_alignment>::type cv;
std::_Aligned_storage_t<Concurrency::details::stl_condition_variable_max_size,
Concurrency::details::stl_condition_variable_max_alignment>
cv;
AZero13 marked this conversation as resolved.
Show resolved Hide resolved

[[nodiscard]] Concurrency::details::stl_condition_variable_interface* _get_cv() noexcept {
// get pointer to implementation
Expand Down
2 changes: 2 additions & 0 deletions stl/src/cthread.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ void _Thrd_yield() { // surrender remainder of timeslice
SwitchToThread();
}

extern "C" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
extern "C" {

_Thrd_equal() is already defined in an extern "C" block, so this is redundant. (See the use of the _EXTERN_C macro above.)

// 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;
Expand All @@ -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();
Expand Down
5 changes: 2 additions & 3 deletions stl/src/iosptrs.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 if-statement in the proposed code, then I'm pretty sure that using the "Step In" function will still allow you to inspect things like the call to DecodePointer. (That's a Win32 function, though, so unless you have the source code, you would just be looking at disassembly.)

No changes are requested from me here.

pf();
}
}
Expand Down
4 changes: 2 additions & 2 deletions stl/src/locale.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link

Choose a reason for hiding this comment

The 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 =
Copy link

Choose a reason for hiding this comment

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

This is also ok.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const auto ptrnewvec =
locale::facet** const ptrnewvec =

This is another case of auto being used when it probably shouldn't be. I like the addition of const to the pointer value itself, though.

static_cast<locale::facet**>(_realloc_crt(_This->_Facetvec, count * sizeof(locale::facet**)));
if (ptrnewvec == nullptr) { // report no memory
_Xbad_alloc();
Expand Down
2 changes: 1 addition & 1 deletion stl/src/locale0.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link

Choose a reason for hiding this comment

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

Why the removal of nullptr here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Presumably, they did that because the ptr variable is immediately initialized to a different value after the _BEGIN_LOCK(_LOCK_LOCALE) macro, so the initialization to nullptr is ultimately redundant. I do like to believe that any optimizing compiler with half of a brain would remove that assignment anyways, though.

(On another note, I really don't like those _BEGIN_LOCK and _END_LOCK macros. I mean, _END_LOCK is literally defined as just an end brace character, for crying out loud! Perhaps a separate PR should be make just to remove those macros, unless there's some really strange reason for its continued existence.)


_BEGIN_LOCK(_LOCK_LOCALE) // prevent double initialization

Expand Down
11 changes: 5 additions & 6 deletions stl/src/multprec.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link

Choose a reason for hiding this comment

The 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 --qh; (was it a bug or something?).

unsigned long long u[2];
unsigned long long v[2];
u[0] = u0 & mask;
Expand All @@ -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;
Expand Down Expand Up @@ -170,7 +170,6 @@ void __CLRCALL_PURE_OR_CDECL _MP_Rem(
}
// D5: [Test remainder.]
if (k != 0) { // D6: [Add back.]
--qh;
Copy link
Contributor

Choose a reason for hiding this comment

The 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.]
Expand Down
5 changes: 3 additions & 2 deletions stl/src/mutex.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link

Choose a reason for hiding this comment

The 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
Expand Down
2 changes: 1 addition & 1 deletion stl/src/nothrow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,5 +14,5 @@
#include <new>
_STD_BEGIN

const nothrow_t nothrow = nothrow_t(); // define nothrow
constexpr nothrow_t nothrow = nothrow_t(); // define nothrow
_STD_END
8 changes: 3 additions & 5 deletions stl/src/pplerror.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,12 +46,10 @@ namespace Concurrency {

#else // defined(_CRT_APP) || defined(UNDOCKED_WINDOWS_UCRT)

namespace Concurrency {
namespace details {
namespace Concurrency::details {
Copy link

Choose a reason for hiding this comment

The 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)
Loading