Closed Bug 467948 Opened 12 years ago Closed 11 years ago

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

Categories

(Core :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dholbert, Unassigned)

References

()

Details

Attachments

(3 files, 5 obsolete files)

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)
Attached patch patch (obsolete) — Splinter Review
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)
Attachment #379577 - Flags: review?(dbaron)
> -  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)
(Thanks for looking into fixing these, by the way! Less compiler spam is awesome.)
Attached patch patch, v2 (obsolete) — Splinter Review
fair enough. Also found some other cases inside that TestUConv file.
Attachment #379577 - Attachment is obsolete: true
Attachment #379659 - Flags: review?(dbaron)
Attachment #379577 - Flags: review?(dbaron)
Attachment #379659 - Flags: review?(dbaron) → review+
Windows tryserver didn't like one of the changes. Still waiting for the tree to open up.
Attachment #379659 - Attachment is obsolete: true
Attachment #382123 - Flags: review+
Comment on attachment 382123 [details] [diff] [review]
patch, v3 [pushed: comment 7]

http://hg.mozilla.org/mozilla-central/rev/96c833c60acb
Attachment #382123 - Attachment description: patch, v3 → patch, v3 [pushed: comment 7]
Depends on: 497832
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.
Attached patch patch, round 2 (obsolete) — Splinter Review
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.
Attachment #384185 - Flags: review?(dbaron)
Attachment #384185 - Flags: review?(dbaron) → review-
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.
(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.
Attachment #384196 - Flags: review?
Attachment #384196 - Flags: review? → review?(dbaron)
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.
Attachment #384185 - Attachment is obsolete: true
Attachment #384196 - Attachment is obsolete: true
Attachment #384206 - Flags: review?(dbaron)
Attachment #384196 - Flags: review?(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
Attachment #384206 - Flags: review?(dbaron) → review+
I updated the patch to tip and added a couple Mac files. Also made the change suggested in comment #13
Attachment #384206 - Attachment is obsolete: true
Attachment #392124 - Flags: review?
Attachment #392124 - Flags: review? → review?(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
Attachment #392124 - Flags: review?(dbaron) → review+
Keywords: checkin-needed
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.
Status: NEW → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Attachment #392124 - Attachment description: Patch, round 2b (updated to tip and added a couple Mac files) → Patch, round 2b (updated to tip and added a couple Mac files) [pushed: comment 16]
You need to log in before you can comment on or make changes to this bug.