Closed Bug 824885 Opened 7 years ago Closed 7 years ago

Fix -Wdeprecated-writable-strings warnings in xremote code

Categories

(Toolkit :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: njn, Assigned: njn)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

clang 3.2 warns:

  toolkit/components/remote/nsXRemoteService.cpp:62:3 [-Wdeprecated-writable-strings] conversion from string literal to 'char *' is deprecated
  toolkit/components/remote/nsXRemoteService.cpp:63:3 [-Wdeprecated-writable-strings] conversion from string literal to 'char *' is deprecated
  toolkit/components/remote/nsXRemoteService.cpp:64:3 [-Wdeprecated-writable-strings] conversion from string literal to 'char *' is deprecated
  toolkit/components/remote/nsXRemoteService.cpp:65:3 [-Wdeprecated-writable-strings] conversion from string literal to 'char *' is deprecated
  toolkit/components/remote/nsXRemoteService.cpp:66:3 [-Wdeprecated-writable-strings] conversion from string literal to 'char *' is deprecated
  toolkit/components/remote/nsXRemoteService.cpp:67:3 [-Wdeprecated-writable-strings] conversion from string literal to 'char *' is deprecated
  toolkit/components/remote/nsXRemoteService.cpp:68:3 [-Wdeprecated-writable-strings] conversion from string literal to 'char *' is deprecated
  toolkit/components/remote/nsXRemoteService.cpp:69:3 [-Wdeprecated-writable-strings] conversion from string literal to 'char *' is deprecated
  toolkit/components/remote/nsXRemoteService.cpp:280:22 [-Wdeprecated-writable-strings] conversion from string literal to 'char *' is deprecated
  toolkit/components/remote/nsXRemoteService.cpp:280:38 [-Wdeprecated-writable-strings] conversion from string literal to 'char *' is deprecated

and:

  widget/xremoteclient/XRemoteClient.cpp:84:3 [-Wdeprecated-writable-strings] conversion from string literal to 'char *' is deprecated
  widget/xremoteclient/XRemoteClient.cpp:85:3 [-Wdeprecated-writable-strings] conversion from string literal to 'char *' is deprecated
  widget/xremoteclient/XRemoteClient.cpp:86:3 [-Wdeprecated-writable-strings] conversion from string literal to 'char *' is deprecated
  widget/xremoteclient/XRemoteClient.cpp:87:3 [-Wdeprecated-writable-strings] conversion from string literal to 'char *' is deprecated
  widget/xremoteclient/XRemoteClient.cpp:88:3 [-Wdeprecated-writable-strings] conversion from string literal to 'char *' is deprecated
  widget/xremoteclient/XRemoteClient.cpp:89:3 [-Wdeprecated-writable-strings] conversion from string literal to 'char *' is deprecated
  widget/xremoteclient/XRemoteClient.cpp:90:3 [-Wdeprecated-writable-strings] conversion from string literal to 'char *' is deprecated
  widget/xremoteclient/XRemoteClient.cpp:91:3 [-Wdeprecated-writable-strings] conversion from string literal to 'char *' is deprecated
  widget/xremoteclient/XRemoteClient.cpp:92:3 [-Wdeprecated-writable-strings] conversion from string literal to 'char *' is deprecated
This patch changes the arrays from |char*[]| to |const char*[]|.
Unfortunately casts are needed when they are used, because the called
functions don't specify const (when it seems they should).

The patch also removes XRemoteClient::mMozSupportsCLAtom, because it's unused:

  widget/xremoteclient/XRemoteClient.h:68:18 [-Wunused-private-field] private field 'mMozSupportsCLAtom' is not used
Attachment #695907 - Flags: review?(karlt)
Blocks: buildwarning
Comment on attachment 695907 [details] [diff] [review]
Fix -Wdeprecated-writable-strings warnings in xremote code.

The Xlib API probably predates const in C and hasn't been updated.
This C/C++ mismatch leads to these sort of problems when you turn on these warnings.  I guess the warning could be valuable in some cases, so the cast is fine provided you use const_cast and not the C-style reinterpret cast.

"[ptr] native nsCharPtrArray(char*)" for nsICommandLineRunner is probably a bug if we are going to have these warnings.  Any reason not to fix that?
Attachment #695907 - Flags: review?(karlt) → review-
FWIW some of Xlib uses _Xconst but XInternAtoms doesn't.  It seems only char* uses _Xconst; char** doesn't.
> "[ptr] native nsCharPtrArray(char*)" for nsICommandLineRunner is probably a
> bug if we are going to have these warnings.  Any reason not to fix that?

In nsICommandLineRunner we have this comment:

  /**
   * This method assumes a native character set, and is meant to be called
   * with the argc/argv passed to main(). Talk to bsmedberg if you need to
   * create a command line using other data. argv will not be altered in any
   * way.
   *
   * On Windows, the "native" character set is UTF-8, not the native codepage.
   *
   * @param workingDir The working directory for resolving file and URI paths.
   * @param state      The nsICommandLine.state flag.
   */
  void init(in long argc, in nsCharPtrArray argv,
            in nsIFile workingDir, in unsigned long state);

Since main takes |char**|, and this is documented as not changing the contents, I thought leaving it was reasonable...
New version using const_cast.  I haven't changed nsICommandLineRunner::Init.
From code inspection it looks like doing so would require changing numerous
other files.
Attachment #695916 - Flags: review?(karlt)
Attachment #695907 - Attachment is obsolete: true
Comment on attachment 695916 [details] [diff] [review]
Fix -Wdeprecated-writable-strings warnings in xremote code.

(In reply to Nicholas Nethercote [:njn] from comment #5)
> I haven't changed nsICommandLineRunner::Init.
> From code inspection it looks like doing so would require changing numerous
> other files.

I only noticed nsCommandLine.cpp.  If this had been addressed in bug 633533, then it wouldn't be in our way here.  But I don't really mind.
Attachment #695916 - Flags: review?(karlt) → review+
I tried changing nsICommandLineRunner::Init to take |const char**| and it got a bit ugly.  In particular, I had to use const_cast<char**> when passing the argument to other functions, like gnome_program_init, that take |char**|.

So, due to external APIs, we have to mix |char**| and |const char**| in an unsound way.  Given that, the minimal fix is easiest.
Actually, gnome_program_init is under our control.  But this ends up at execv, which requires a |char *const[]|, weirdly enough.  The APIs are simply against us in this case, so a minimal change will suffice.
https://hg.mozilla.org/mozilla-central/rev/9d29a67f90b6
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.