Last Comment Bug 654618 - nptest.cpp:1084:58: warning: comparison between signed and unsigned integer expressions
: nptest.cpp:1084:58: warning: comparison between signed and unsigned integer e...
Status: RESOLVED FIXED
[build_warning]
:
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: Trunk
: x86_64 Other
: -- normal (vote)
: mozilla18
Assigned To: SADINENI RAVI CHANDRA
:
:
Mentors:
Depends on:
Blocks: buildwarning
  Show dependency treegraph
 
Reported: 2011-05-03 17:08 PDT by Daniel Holbert [:dholbert]
Modified: 2012-09-11 06:58 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
changed the NULL assignment to char variables to '\0' and changed int32_t to uint32_t for width and height (1.50 KB, patch)
2012-08-28 22:10 PDT, SADINENI RAVI CHANDRA
no flags Details | Diff | Splinter Review
made the changes accordingly (1.11 KB, patch)
2012-08-30 03:43 PDT, SADINENI RAVI CHANDRA
no flags Details | Diff | Splinter Review
checked if the value is >= 0 before typecasting (1.25 KB, patch)
2012-09-02 10:28 PDT, SADINENI RAVI CHANDRA
benjamin: review+
Details | Diff | Splinter Review

Description Daniel Holbert [:dholbert] 2011-05-03 17:08:58 PDT
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
}
Comment 1 SADINENI RAVI CHANDRA 2012-08-28 10:25:06 PDT
bug is already fixed...
Comment 2 Daniel Holbert [:dholbert] 2012-08-28 10:55:26 PDT
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]
}
Comment 3 SADINENI RAVI CHANDRA 2012-08-28 21:14:42 PDT
i am sorry ..... i am working on it
Comment 4 SADINENI RAVI CHANDRA 2012-08-28 22:10:53 PDT
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
Comment 5 Daniel Holbert [:dholbert] 2012-08-29 15:28:07 PDT
>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
Comment 6 SADINENI RAVI CHANDRA 2012-08-30 03:43:25 PDT
Created attachment 656806 [details] [diff] [review]
made the changes accordingly
Comment 7 Daniel Holbert [:dholbert] 2012-08-30 07:25:56 PDT
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.)
Comment 8 SADINENI RAVI CHANDRA 2012-09-02 10:28:38 PDT
Created attachment 657694 [details] [diff] [review]
checked if the value is >= 0 before typecasting
Comment 9 Ryan VanderMeulen [:RyanVM] 2012-09-02 10:36:55 PDT
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
Comment 10 Daniel Holbert [:dholbert] 2012-09-10 15:34:45 PDT
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.
Comment 11 Daniel Holbert [:dholbert] 2012-09-10 15:35:46 PDT
(updating bug summary to be about the build warning that the patch is ultimately fixing)
Comment 12 Daniel Holbert [:dholbert] 2012-09-10 23:56:06 PDT
Landed patch:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/c732735ca470
Comment 13 Ed Morley [:emorley] 2012-09-11 06:58:48 PDT
https://hg.mozilla.org/mozilla-central/rev/c732735ca470

Note You need to log in before you can comment on or make changes to this bug.