Closed Bug 467948 Opened 12 years ago Closed 11 years ago
Fix "warning: deprecated conversion from string constant" compiler warnings
54.78 KB, text/plain
17.25 KB, patch
|Details | Diff | Splinter Review|
10.66 KB, patch
|Details | Diff | Splinter Review|
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)
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)
> - 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.)
fair enough. Also found some other cases inside that TestUConv file.
Attachment #379659 - Flags: review?(dbaron) → review+
Windows tryserver didn't like one of the changes. Still waiting for the tree to open up.
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]
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.
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.
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
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+
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
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.