467948 - Fix "warning: deprecated conversion from string constant" compiler warnings

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: list of all 427 warnings

During a compile of mozilla-central, I get 427 instances of this compiler warning:
   warning: deprecated conversion from string constant to ‘char*’

AFAIK, all that's required to fix this is the use of "const char*" instead of "char*" in the right places.

I'm filing this bug on fixing these warnings (where possible), to reduce the amount of needless compiler-spam.

Attached is a list of all these warnings as of today. (with filename/line number next to each warning)

Comment 1

[Arpad Borsos [:Swatinem]]

Attachment: patch

This patch does fix some of those warnings, not all though, as some are hard or even impossible to fix (as some depend on external symbols)

Comment 2

[Daniel Holbert [:dholbert]]

> -  char * trace = "TestEncoders";
> -  mLog.AddTrace(trace);
> +  mLog.AddTrace("TestEncoders");
[snip]  
> -  mLog.DelTrace(trace);
> +  mLog.DelTrace("TestEncoders");

It's not a good idea to type out multiple instances of the same string-literal -- that's just asking for bugs (i.e. accidental misspellings in one instance will mean the trace never gets deleted). I'd recommend keeping the local variable "trace", and just making it a const char* instead of a char*.

(This applies to a number of different places in the patch -- every call to AddTrace / DelTrace)

Comment 3

[Daniel Holbert [:dholbert]]

(Thanks for looking into fixing these, by the way! Less compiler spam is awesome.)

Comment 4

[Arpad Borsos [:Swatinem]]

Attachment: patch, v2

fair enough. Also found some other cases inside that TestUConv file.

Comment 5

[David Baron :dbaron:]

Comment on attachment 379659 [details] [diff] [review]
patch, v2

r=dbaron

Comment 6

[Arpad Borsos [:Swatinem]]

Attachment: patch, v3 [pushed: comment 7]

Windows tryserver didn't like one of the changes. Still waiting for the tree to open up.

Comment 7

[Arpad Borsos [:Swatinem]]

Comment on attachment 382123 [details] [diff] [review]
patch, v3 [pushed: comment 7]

http://hg.mozilla.org/mozilla-central/rev/96c833c60acb

Comment 8

[Arpad Borsos [:Swatinem]]

According to bsmedbergs warnings tracker, there are still 27 unique warnings left.
Some of those can't really be "fixed" just "worked around". In cases where an API function takes a "char * const *" (that is, a const pointer to a non-const pointer to non-const chars) instead of a "const char const * const *" (where each element is const) only a const_cast will help.

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: patch, round 2

Using stock mozilla-central, I still get 65 instances of this warning.  The attached patch fixes these remaining instances (using const_cast in some cases).

const_cast is admittedly a little messy, but I think this patch is still better than the status quo.  In current mozilla-central, we're effectively forcing the compiler to do a const_cast under the hood (and to complain about it) -- this patch just makes those conversions explicit.

Comment 10

[David Baron :dbaron:]

Comment on attachment 384185 [details] [diff] [review]
patch, round 2

Using const_cast is the wrong solution; we should be marking the necessary strings as |const char*| rather than just |char*|.  If that's hard, we should live with the warning for now.

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: patch, round 2b (removed const_cast parts)

(In reply to comment #10)
> (From update of attachment 384185 [details] [diff] [review])
> we should be marking the necessary
> strings as |const char*| rather than just |char*|.

Agreed -- but I think most of the remaining sources of this warning would classify as "hard", then, because e.g. the string literal is passed as a "char*" argument through a (frozen?) interface (e.g. nsIProcess, nsICommandLineRunner) or into an external library (e.g. "XInternAtoms" in Xlib).  For those, our options would be to
  a) change the API (which I think is undesirable and/or impossible in many cases)
  b) use const_cast just before making the API call
  c) live with the warnings

I guess we'll go with (c) for these situations, for now, then.

Here's here's a stripped-down version of the "round 2" patch, with the const_cast chunks removed.

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: patch, round 2b (removed const_cast parts & changes to nsNSSCertificateDB.cpp)

Actually, I just found out from the Try Server that the one-line change in nsNSSCertificateDB.cpp causes a legitimate build failure on Windows.

Apparently on most platforms (Linux, Maemo, and WinCE Try Server builders so far), the 'strchr' method will happily (and incorrectly, I think) return a NON-CONST char* pointer that points into a const char* buffer.  But on Windows, that doesn't work -- it causes a build failure, with my old fix for nsNSSCertificateDB.cpp.

So, this patch fixes the warning in that file slightly differently.

Comment 13

[David Baron :dbaron:]

Comment on attachment 384206 [details] [diff] [review]
patch, round 2b (removed const_cast parts & changes to nsNSSCertificateDB.cpp)

>-      if (!namestr) namestr = "";
>-      nsAutoString certname = NS_ConvertASCIItoUTF16(namestr);
>+      const char* nonNullNamestr = namestr ? namestr : "";
>+      nsAutoString certname = NS_ConvertASCIItoUTF16(nonNullNamestr);

Might be better to just put namestr ? namestr : "" inside the NS_ConvertASCIItoUTF16, if you can.

r=dbaron

Comment 14

[Craig Topper]

Attachment: Patch, round 2b (updated to tip and added a couple Mac files) [pushed: comment 16]

I updated the patch to tip and added a couple Mac files. Also made the change suggested in comment #13

Comment 15

[David Baron :dbaron:]

Comment on attachment 392124 [details] [diff] [review]
Patch, round 2b (updated to tip and added a couple Mac files) [pushed: comment 16]

r=dbaron

Comment 16

[Daniel Holbert [:dholbert]]

Landed: http://hg.mozilla.org/mozilla-central/rev/a61665851093
(Craig: Thanks for the patch update!)

Closing this bug, since I think most of the remaining instances of this warning are from frozen-ish interfaces, per comment 11.