824885 - Fix -Wdeprecated-writable-strings warnings in xremote code

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Nicholas Nethercote [inactive]]

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

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Fix -Wdeprecated-writable-strings warnings in xremote code.

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

Comment 2

[Karl Tomlinson (:karlt)]

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?

Comment 3

[Karl Tomlinson (:karlt)]

FWIW some of Xlib uses _Xconst but XInternAtoms doesn't.  It seems only char* uses _Xconst; char** doesn't.

Comment 4

[Nicholas Nethercote [inactive]]

> "[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...

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: Fix -Wdeprecated-writable-strings warnings in xremote code.

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.

Comment 6

[Karl Tomlinson (:karlt)]

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.

Comment 7

[Nicholas Nethercote [inactive]]

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.

Comment 8

[Nicholas Nethercote [inactive]]

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.

Comment 9

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d29a67f90b6

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9d29a67f90b6