nptest.cpp:1084:58: warning: comparison between signed and unsigned integer expressions

RESOLVED FIXED in mozilla18

Status

()

Core
Plug-ins
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: dholbert, Assigned: SADINENI RAVI CHANDRA)

Tracking

(Blocks: 1 bug)

Trunk
mozilla18
x86_64
Other
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [build_warning])

Attachments

(1 attachment, 2 obsolete attachments)

nptest.cpp currently issues 7 lines of build warnings when built with g++-4.5.  

They all look to be worth fixing:
{
> modules/plugin/test/testplugin/nptest.cpp:315:1: warning: converting to non-pointer type 'uint32_t' from NULL
> modules/plugin/test/testplugin/nptest.cpp: In function 'int32_t NPP_Write(NPP_t*, NPStream*, int32_t, int32_t, void*)':
> modules/plugin/test/testplugin/nptest.cpp:1175:15: warning: unused variable 'err'
> modules/plugin/test/testplugin/nptest.cpp: In function 'bool setSitesWithData(NPObject*, const NPVariant*, uint32_t, NPVariant*)':
> modules/plugin/test/testplugin/nptest.cpp:3412:26: warning: converting to non-pointer type 'char' from NULL
> modules/plugin/test/testplugin/nptest.cpp:3414:27: warning: converting to non-pointer type 'char' from NULL
> modules/plugin/test/testplugin/nptest.cpp:3415:23: warning: converting to non-pointer type 'char' from NULL
}
Summary: nptest.cpp build warnings: "converting to non-pointer type 'uint32_t' from NULL", "unused variable 'err'", & 3x" converting to non-pointer type 'char' from NULL" → nptest.cpp build warnings: "converting to non-pointer type 'uint32_t' from NULL", "unused variable 'err'", & 3x "converting to non-pointer type 'char' from NULL"
Blocks: 187528
Assignee: nobody → ravicat2013
Status: NEW → ASSIGNED
(Assignee)

Comment 1

5 years ago
bug is already fixed...
I still get the last 3 warnings from comment 0, when compiling that file (along with 2 new "comparison between signed and unsigned" warnings):
{
dom/plugins/test/testplugin/nptest.cpp: In function ‘NPError NPP_SetWindow(NPP, NPWindow*)’:
dom/plugins/test/testplugin/nptest.cpp:1084:58: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
dom/plugins/test/testplugin/nptest.cpp:1085:59: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
dom/plugins/test/testplugin/nptest.cpp: In function ‘bool setSitesWithData(NPObject*, const NPVariant*, uint32_t, NPVariant*)’:
dom/plugins/test/testplugin/nptest.cpp:3595:26: warning: converting to non-pointer type ‘char’ from NULL [-Wconversion-null]
dom/plugins/test/testplugin/nptest.cpp:3597:27: warning: converting to non-pointer type ‘char’ from NULL [-Wconversion-null]
dom/plugins/test/testplugin/nptest.cpp:3598:23: warning: converting to non-pointer type ‘char’ from NULL [-Wconversion-null]
}
(Assignee)

Comment 3

5 years ago
i am sorry ..... i am working on it
(Assignee)

Comment 4

5 years ago
Created attachment 656331 [details] [diff] [review]
changed the NULL assignment to char variables to '\0' and changed int32_t to uint32_t for width and height
Attachment #656331 - Flags: review?(dholbert)
>diff -r 118cc431d56f dom/plugins/base/npapi.h
>--- a/dom/plugins/base/npapi.h	Mon Aug 27 21:34:53 2012 -0700
>+++ b/dom/plugins/base/npapi.h	Wed Aug 29 10:33:56 2012 +0530
> typedef struct _NPSize
> {
>-  int32_t width;
>-  int32_t height;
>+  uint32_t width;
>+  uint32_t height;
> } NPSize;
> 

We probably shouldn't be changing actual code / APIs just to fix a build-warning in test code. I don't know why NPSize is currently signed -- I don't know our plugins API -- but I'd assume that there's a reason it's the way it is (e.g. to indicate errors in some cases), and changing it like your patch does would probably have additional implications.

So: I suspect we probably want to fix those signed/unsigned warnings in the test, rather than in the API that the test.  We probably want a bounds-check and a cast -- e.g. convert from this...
  if (signedVal == unsignedVal)
...to this...
  if (signedVal >= 0 &&
      ((uint32_t)signedVal) == unsignedVal)

Hopefully that makes sense.

>diff -r 118cc431d56f dom/plugins/test/testplugin/nptest.cpp
>     // Parse out the three tokens into a siteData struct.
>     const char* siteEnd = strchr(iterator, ':');
>-    *((char*) siteEnd) = NULL;
>+    *((char*) siteEnd) = '\0';
>     const char* flagsEnd = strchr(siteEnd + 1, ':');
>-    *((char*) flagsEnd) = NULL;
>-    *((char*) next) = NULL;
>+    *((char*) flagsEnd) = '\0';
>+    *((char*) next) = '\0';

This part looks good!

Also: You should ask for review from someone who's reviewed changes to dom/plugins before -- probably joshmoz or :bsmedberg
OS: Linux → Other
(Assignee)

Comment 6

5 years ago
Created attachment 656806 [details] [diff] [review]
made the changes accordingly
Attachment #656331 - Attachment is obsolete: true
Attachment #656331 - Flags: review?(dholbert)
Attachment #656806 - Flags: review?(benjamin)
Comment on attachment 656806 [details] [diff] [review]
made the changes accordingly

>diff -r 118cc431d56f dom/plugins/test/testplugin/nptest.cpp
>   if (instanceData->asyncDrawing == AD_BITMAP) {
>     if (instanceData->frontBuffer &&
>-        instanceData->frontBuffer->size.width == window->width &&
>-        instanceData->frontBuffer->size.height == window->height) {
>+        (uint32_t)instanceData->frontBuffer->size.width == window->width &&
>+        (uint32_t)instanceData->frontBuffer->size.height == window->height) {
>           return NPERR_NO_ERROR;

(Technically you should be range-checking the signed integers (checking that they're >=0) before casting them to unsigned, as I suggested in comment 5.  Otherwise, the cast is potentially bogus.)
(Assignee)

Comment 8

5 years ago
Created attachment 657694 [details] [diff] [review]
checked if the value is >= 0 before typecasting
Attachment #656806 - Attachment is obsolete: true
Attachment #656806 - Flags: review?(benjamin)
(Assignee)

Updated

5 years ago
Attachment #657694 - Flags: review?(benjamin)
Sadineni, to make life easier for those who will eventually be checking in your patch, can you make sure it follows the guidelines shown below? Thanks!
https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in
Attachment #657694 - Flags: review?(benjamin) → review+
The second hunk of this patch no longer applies, because it was fixed separately in bug 787730.

The first hunk (for signed/unsigned comparisons) still applies, though.  I'll land that today or tomorrow.
(updating bug summary to be about the build warning that the patch is ultimately fixing)
Summary: nptest.cpp build warnings: "converting to non-pointer type 'uint32_t' from NULL", "unused variable 'err'", & 3x "converting to non-pointer type 'char' from NULL" → nptest.cpp:1084:58: warning: comparison between signed and unsigned integer expressions
Landed patch:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/c732735ca470
https://hg.mozilla.org/mozilla-central/rev/c732735ca470
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.