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

PlatformDefs.h: remove DWORD define #17369

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lrusak
Copy link
Contributor

@lrusak lrusak commented Feb 12, 2020

more cleanup of PlatformDefs.h

This is just a global replace of DWORD to uin32_t. I'll probably have to fix up some windows things as it compiles fine on linux.

find xbmc -type f | xargs sed -i 's/DWORD/uint32_t/g'

@lrusak lrusak added Type: Cleanup non-breaking change which removes non-working or unmaintained functionality v19 Matrix labels Feb 12, 2020
@lrusak lrusak added this to the Matrix 19.0-alpha 1 milestone Feb 12, 2020
Copy link
Member

@AlwinEsch AlwinEsch left a comment

Choose a reason for hiding this comment

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

To have them in C++11 style is fine, only was by your sed some another places where contains DWORD (e.g. DWORD64, DWORD_SIZE or own defines) changed.
Also I'm not complete sure, if on calls to Windows it is always OK, if they have DWORD as function value?
Further are there also places with e.g. BYTE in same line, then it look a bit mixed.

#ifndef LODWORD
#define LODWORD(longval) ((DWORD)((DWORDLONG)(longval)))
#ifndef LOuint32_t
#define LOuint32_t(longval) ((uint32_t)((uint32_tLONG)(longval)))
Copy link
Member

Choose a reason for hiding this comment

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

Should this DWORDLONG not a uint64_t and the the LODWORD not only uint32_t (the counterpart HIDWORD is not used)? Maybe better to leave the define or to change the using code places?

Was confused 😄 and also not found something on web about LOuint32_t or uint32_tLONG

@@ -265,7 +273,7 @@ void DXT4toARGB(const void *src, void *dest, unsigned int destWidth)
// and assign them to our texture
for (int y = 0; y < 4; y++)
{
DWORD *d = (DWORD *)dest + destWidth * y;
uint32_t* d = (uint32_t*)dest + destWidth * y;
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense here (in separate commit) set it to uint32_t* d = reinterpret_cast<uint32_t*>(dest + destWidth * y);, here and on other places?

Not directly in the sense of the request, only heard frequently that on change it should then change to C++ style also.

ping @Rechi

@@ -213,7 +214,7 @@ ssize_t CWin32File::Read(void* lpBuf, size_t uiBufSize)
// number of bytes than requested, just return number of read bytes (work as
// transparent wrapper for ReadFile() ).
// If ReadFile() can't read any more bytes than don't try to fill buffer.
if (uiBufSize <= DWORD_MAX || lastRead == 0)
if (uiBufSize <= uint32_t_MAX || lastRead == 0)
Copy link
Member

Choose a reason for hiding this comment

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

The uint32_t_MAX also not present variable (on several places), better the to use UINTMAX_MAX (http://www.cplusplus.com/reference/cstdint/)

@@ -226,8 +237,8 @@ bool win32_exception::write_stacktrace(EXCEPTION_POINTERS* pEp)
{
if(frame.AddrPC.Offset != 0)
{
DWORD64 symoffset=0;
DWORD lineoffset=0;
uint32_t64 symoffset = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Also here wrongly changed from sed

#define IMAGE_SYM_uint32_t_NULL 0
#define IMAGE_SYM_uint32_t_POINTER 1
#define IMAGE_SYM_uint32_t_FUNCTION 2
#define IMAGE_SYM_uint32_t_ARRAY 3
Copy link
Member

Choose a reason for hiding this comment

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

This defines also not direct correct 😄

@jenkins4kodi jenkins4kodi added the Rebase needed PR that does not apply/merge cleanly to current base branch label Feb 18, 2020
@jenkins4kodi
Copy link
Contributor

@lrusak this needs a rebase

@phunkyfish phunkyfish added PR Cleanup: Ask Author PR appears to still be relevant and mergeable. Comment to author to check on status. PR Cleanup: Recent Checked as part of PR cleanup. PR has been followed up on recently. and removed PR Cleanup: Ask Author PR appears to still be relevant and mergeable. Comment to author to check on status. labels Mar 10, 2020
@DaveTBlake DaveTBlake added v20 Nexus and removed PR Cleanup: Recent Checked as part of PR cleanup. PR has been followed up on recently. v19 Matrix labels Oct 30, 2020
@DaveTBlake DaveTBlake removed this from the Matrix 19.0-alpha 3 milestone Oct 30, 2020
@fuzzard fuzzard removed the v20 Nexus label Oct 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Rebase needed PR that does not apply/merge cleanly to current base branch Type: Cleanup non-breaking change which removes non-working or unmaintained functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants