Intrusiveness minimizing changes to suppress warnings for build inside NuttX tree

[ppisa]

@ghaerr @Acfboy @acassis I have prepared changes which suppress all warnings in NuttX build including Microwindows and Nano-X demos except the last two

CC:  nxcalc.c nxcalc.c: In function ‘press_button’:
nxcalc.c:383:42: warning: ‘snprintf’ output may be truncated before the last format character [-Wformat-truncation=]
  383 |           snprintf(tmp, sizeof(tmp), "-%s", display);
      |                                          ^
nxcalc.c:383:11: note: ‘snprintf’ output between 2 and 129 bytes into a destination of size 128
  383 |           snprintf(tmp, sizeof(tmp), "-%s", display);
      |           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
CC:  microwindows/src/nanox/client.c microwindows/src/nanox/client.c: In function ‘GrQueryPointer’:
microwindows/src/nanox/client.c:4497:28: warning: variable ‘req’ set but not used [-Wunused-but-set-variable]
 4497 |         nxQueryPointerReq *req;
      |                            ^~~

The second one is easy to fix as well but it is question if they should not be investigated the first to decide whether they should not stay or be replaced by TODO.

If the changes are acceptable and accepted, then the new SHA for Microwindows should be updated in

apache/nuttx-apps#3768

and it should open door to merge documentation and configurations

apache/nuttx#19912

I have tested build for qemu-intel64 and did not test the result even for functionality till now. It should be tested on some 32-bit NuttX target as well and should be checked to not make problems for other non-NuttX active targets.

[ghaerr]

Hello @ppisa,

This looks pretty good, although things could get a little tricky with UINT_PTR. See the following in include/windef.h:

// LONG_PTR definition, must hold int, long and pointer
#if WIN64_PORT
// definition for 64-bit Microsoft Windows port (int=32, long=32, ptr=64)
typedef __int64             LONG_PTR;   // must hold long and pointer
typedef unsigned __int64    ULONG_PTR;  // must hold unsigned long and pointer
typedef unsigned __int64    UINT_PTR;   // must hold unsigned int and pointer
#else
// definition for 64bit port (int=32, long=64, ptr=64)
//             or 32bit port (int=32, long=32, ptr=32)
typedef long                LONG_PTR;   // must hold long and pointer
typedef unsigned long       ULONG_PTR;  // must hold unsigned long and pointer
typedef unsigned long       UINT_PTR;   // must hold unsigned int and pointer
// smaller definition for 16bit WPARAM (int=16, long=32, ptr=16)
//typedef unsigned int      UINT_PTR;   // holds int and pointer
#endif

I'm not sure whether you're aware of the WIN64_PORT define requirement for UINT_PTR to work properly. Ultimately, one could use <stdint.h> and uintptr_t, but that has its own portability issues with older systems. I was then a bit worried about ELKS (a 16-bit system), using the above definition (e.g. WIN64_PORT=0) which then would set UINT_PTR to unsigned long which is 32-bits (incorrect).

But then I ran into PTR_IS_ATOM() later in windef.h:

#define PTR_IS_ATOM(ptr)        ((((UINT_PTR)ptr) & ~(UINT_PTR)0xFFFFL) == 0)   /* any bits 16+ and higher zero*/

and realized 16-bit systems won't run the Microwindows Win32 API without more issues.

So I think we're OK for now with your usage of UINT_PTR.

I have tested build for qemu-intel64 and did not test the result even for functionality till now. It should be tested on some 32-bit NuttX target as well

Yes, given the use of UINT_PTR and WIN64_PORT above, this should probably be tested on 32- and 64-bit systems. Overall the changes should not be a big effect since in all cases they are also further casted to UINT, which I assume is 32-bits even on 64-bit systems (thus nullifying somewhat the issue of WIN64_PORT above).

nxcalc.c:383:42: warning: ‘snprintf’ output may be truncated before the last format character [-Wformat-truncation=]
383 | snprintf(tmp, sizeof(tmp), "-%s", display);

This is probably easiest fixed by just changing the size of char tmp[128] to char tmp[130] in the function calling snprintf.

/src/nanox/client.c:4497:28: warning: variable ‘req’ set but not used [-Wunused-but-set-variable]
4497 | nxQueryPointerReq *req;

If you want, remove the "req = " in the following line and add a comment saying return value ignored:

    req = AllocReq(QueryPointer);

or perhaps just use (void)req after the allocation?

I have one other question which I'll post next to the code changed.

Thank you!

[ghaerr]

Why is line 806 required? rcClient is used directly above and below, so not sure why this is needed. What was the warning given by the compiler?

[ppisa]

The structure fields are setup

rcClient.left = 0;
...

but it is not used in the latter code for now. So warning is correct.

The code documents intention but then ends with commented call because WM_SIZECHANGED, seems to be unimplemented for now or it is implemented but already but change did not propagate back into winsbar.c

//SendMessage(hWnd, WM_SIZECHANGED, (WPARAM)&rcWin, (LPARAM)&rcClient);

[ppisa]

I am aware of 64-bit widows (WIN64_PORT) use of non-standard LLP64 breaking Unix prerequisite that long can hold pointer. But my change only convert the pointer to marching size unsigned integer and then it is truncated implicitly or by (int) or (DWORD) casts. The conversion of pointer to UINT_PTR has no real effect but it removes warning because latter implicit typecast to narrower integer is considered as intentional.

I am in some doubts, if some conversions to int should not be to DWORD in the fact. It depend on how the field, parameter of pointer type is reused and filled. I have not checked when the code calls functions affected. But there should not be any real operation change by casts I have added.

As for unit_ptr_t, I would preffer that and it would be ideal if UINT_PTR is set to this type everywhere but I expect that there can be some old compilers and environments where it can be a problem. So I hav stuck with type, which is guaranteed to be available in WinAPI world.

[ghaerr]

Thanks for your comments @ppisa. I am OK with committing these changes when you indicate to proceed.

[ppisa]

Great, I leave some time to @Acfboy and others to do more tests if we have not broken something. I have tested the changes in my qemu-intel64 environment that it runs OK.

[ppisa]

I try to test something to silence snprintf and if you think that silence of unused req is OK for now, then I add

(void)req;

there.

[ppisa]

OK, I have silenced nxQueryPointerReq *req; in nanox/client.c and then found that the snprintf is in nxcalc.c copy in nuttx/apps examples. I have problem to find reasonable silence trick with snprintf so I switched to memmove, see preliminary commit for nuttx-apps ppisa/nuttx-apps@389d96c.

So I think that with this Microwindows commit and then updated nuttx-apps we are getting to state when Micowidows config and documentation could be accepted into mainline.

[ghaerr]

Sounds good, let me know when you want committed.

[ppisa]

Thanks. If you do not see anything suspicious and nobody else comes with something then I am for committing.