Closed
Bug 1003902
Opened 10 years ago
Closed 10 years ago
mozStorageStatement.h(37) : warning C4373: 'mozilla::storage::Statement::EscapeStringForLIKE': virtual function overrides 'mozilla::storage::StorageBaseStatementInternal::EscapeStringForLIKE', previous versions of the compiler did not override when [...]
Categories
(Toolkit :: Storage, defect)
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: dholbert, Assigned: dholbert)
References
(Blocks 2 open bugs)
Details
Attachments
(2 files, 2 obsolete files)
2.32 KB,
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
1.10 KB,
text/plain
|
Details |
We get 4 copies of this pair of build warnings, in Windows builds: { storage\src\mozStorageStatement.h(37) : warning C4373: 'mozilla::storage::Statement::EscapeStringForLIKE': virtual function overrides 'mozilla::storage::StorageBaseStatementInternal::EscapeStringForLIKE', previous versions of the compiler did not override when parameters only differed by const/volatile qualifiers storage\src\mozStorageAsyncStatement.h(36) : warning C4373: 'mozilla::storage::AsyncStatement::EscapeStringForLIKE': virtual function overrides 'mozilla::storage::StorageBaseStatementInternal::EscapeStringForLIKE', previous versions of the compiler did not override when parameters only differed by const/volatile qualifiers } In the classes being warned about (Statement & AsyncStatement), this method is declared via the macro NS_DECL_MOZISTORAGEBASESTATEMENT, which comes from this IDL: >151 AString escapeStringForLIKE(in AString aValue, in wchar aEscapeChar); http://mxr.mozilla.org/mozilla-central/source/storage/public/mozIStorageBaseStatement.idl#151 ...which produces this generated signature in the .h file: > NS_IMETHOD EscapeStringForLIKE(const nsAString & aValue, char16_t aEscapeChar, nsAString & _retval) = 0; In the parent class mentioned in the warning -- StorageBaseStatementInternal -- the function decl is hardcoded: > 143 NS_IMETHOD EscapeStringForLIKE(const nsAString &aValue, > 144 const char16_t aEscapeChar, > 145 nsAString &_escapedString); http://mxr.mozilla.org/mozilla-central/source/storage/src/StorageBaseStatementInternal.h#143 It looks like the hardcoded function-signature incorrectly has "const" on aEscapeChar" -- that's the mismatch.
Assignee | ||
Comment 1•10 years ago
|
||
hg archeology shows that this has been an issue ever since bug 507414 added these files (mozIStorageBaseStatement.idl & StorageBaseStatementInternal.h).
Depends on: 507414
Assignee | ||
Comment 2•10 years ago
|
||
Here's a patch to drop this 'const'. Haven't tested yet; will request review when I do & confirm that this fixes the warning.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•10 years ago
|
||
I actually don't get this warning in my local windows build (using MSVC 2012). I only see it in TBPL logs. Maybe it's disabled in newer MSVC versions? Anyway, here's a Try run *without* this bug's fix, and with the directory annotated as FAIL_ON_WARNINGS, which I'm expecting to run afoul of this and be red: https://tbpl.mozilla.org/?tree=Try&rev=9f92ecd046b4 (expected to be red) ...and here's a Try run with the same patches, *plus* this bug's fix: https://tbpl.mozilla.org/?tree=Try&rev=912bda3beb6f (expected to be green)
Assignee | ||
Comment 4•10 years ago
|
||
(er, attached wrong patch file before. here's the right one.)
Attachment #8415364 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8415364 -
Attachment description: fix v1 → (Wrong patch file)
Assignee | ||
Comment 5•10 years ago
|
||
Comment on attachment 8415380 [details] [diff] [review] fix v1 Requesting review. The first Try run already failed from this warning; the second (with this bug's fix) is still going (on Windows), which I think indicates that this fixes it.
Attachment #8415380 -
Flags: review?(mak77)
Comment 6•10 years ago
|
||
Comment on attachment 8415380 [details] [diff] [review] fix v1 Review of attachment 8415380 [details] [diff] [review]: ----------------------------------------------------------------- I see this warning with MSVC2013, not sure what differs from your setup... btw, here we have a proxying implementation that declares NS_IMETHODIMP (Statement|AsyncStatement)EscapeStringForLIKE(const nsAString &aValue, const char16_t aEscapeChar, nsAString &_escapedString) http://mxr.mozilla.org/mozilla-central/source/storage/src/StorageBaseStatementInternal.h#190 this proxies to the internal implementation that is in http://mxr.mozilla.org/mozilla-central/source/storage/src/StorageBaseStatementInternal.cpp#202 though looks like now the signature and the implementation for the internal class don't match, I wonder if tests would fail (the try build only has builds)... All of this setup is confusing me (and I guess not just me), maybe we should rename the internal proxyfied-to methods and make it a little bit more explicit, the internal method could still use const, while the proxy should stick to the interface... or at least, this is what I seem to understand (this code predates me) I guess renaming them to SharedEscapeStringForLIKE for example (the MIX_IMPL define may just add a Shared before _method I guess), could help unbundling this...
Assignee | ||
Comment 7•10 years ago
|
||
(In reply to Marco Bonardo [:mak] from comment #6) > I see this warning with MSVC2013, not sure what differs from your setup... Oh... d'oh, I think I was probably just wrong. (I was testing for whether I got it based on "does a FAIL_ON_WARNINGS annotation break my build?"), but I actually probably built [locally] without --enable-warnings-as-errors, since I rarely boot into windows and probably have a trivial mozconfig there.) Anyway, this has now been Try-tested, so my local testing isn't relevant anyway. > btw, here we have a proxying implementation that declares > NS_IMETHODIMP (Statement|AsyncStatement)EscapeStringForLIKE(const nsAString > &aValue, const char16_t aEscapeChar, nsAString &_escapedString) > http://mxr.mozilla.org/mozilla-central/source/storage/src/ > StorageBaseStatementInternal.h#190 Thanks, I'll fix that macro, too. > I guess renaming them to SharedEscapeStringForLIKE for example (the MIX_IMPL > define may just add a Shared before _method I guess), could help unbundling > this... I'd like to keep this bug/patch narrowly-scoped & avoid changing behavior, if that's OK. If we want to refactor this code to split off a renamed helper-function from the actual public function, could that happen elsewhere? (I don't think it's too important (from a safety standpoint) that we make this char16_t arg be const; we have tons of functions that take a bool, or a nscoord, or a uint_* flags arg, which the function doesn't modify, and we rarely bother labeling those args as const. So I think it's simpler to just make it match the IDL-generated header.)
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8415380 -
Attachment is obsolete: true
Attachment #8415380 -
Flags: review?(mak77)
Attachment #8415550 -
Flags: review?(mak77)
Comment 9•10 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #7) > I'd like to keep this bug/patch narrowly-scoped & avoid changing behavior, > if that's OK. If we want to refactor this code to split off a renamed > helper-function from the actual public function, could that happen elsewhere? the fact is that I'm not comfortable reviewing the change with the current state of the code, those overloads are crazy and we might introduce subtle bugs with this stuff proxying to a same-name method, where the only difference is one argument being const or not. That's why I suggested renaming the base methods and fixing the proxy declarations, that should be cleaner and safer (the compiler can help figuring issues better than I can at that point)
Assignee | ||
Comment 10•10 years ago
|
||
The build warning is actually telling us that, even though the function signatures *currently don't match*, the override *does* succeed, though it wouldn't have in previous MSVC versions. So the 'const' removal (making the function-signatures match) doesn't affect the overriding success/failure here -- it continues to succeed. (It *would* affect things if this were changing us from a const to a non-const *method*. But if we're changing the const-ness of args, we can still successfully override (in clang++ and g++, according to local testing, and in MSVC according to the warning).) Moreover, even if I were wrong about this and this introduced a bug where the overload stopped working, compilers would warn us about that -- they're good about that -- and FAIL_ON_WARNINGS would catch that.
Assignee | ||
Comment 11•10 years ago
|
||
This C++ testcase illustrates that adding/removing 'const' from an arg doesn't affect overriding. (though adding/removing 'const' from the end of the method does) g++ and clang++ both produce the following results when I compile & run: { Does child's override succeed, adding 'const' to arg? yes (this is ChildAddingConstToArg) Does child's override succeed, dropping 'const' from arg? yes (this is ChildDroppingConstFromArg) Does child's override succeed, adding 'const' to method? NO! OH NOES (this is parent) } Moreover, clang warns with this output for the third (failing-to-override) child class: { foo.cpp:25:16: warning: 'ChildAddingConstToMethod::DoStuff' hides overloaded virtual function [-Woverloaded-virtual] virtual void DoStuff(int a, const int b) const { ^ foo.cpp:4:16: note: hidden overloaded virtual function 'Parent::DoStuff' declared here: different qualifiers (none vs const) virtual void DoStuff(int a, const int b) { ^ 1 warning generated. }
Assignee | ||
Comment 12•10 years ago
|
||
So, the patch (a) does not affect behavior (per comment 10 / 11, and per the text of the actual MSVC warning) (b) gets us to more technically-correct (c) fixes a build warning :mak, does that sway you? :)
Flags: needinfo?(mak77)
Assignee | ||
Comment 13•10 years ago
|
||
(and (d) lets us mark the directory as FAIL_ON_WARNINGS which will let clang enforce that we don't have any near-miss function overrides like the hypothetical ones you're concerned about introducing, in comment 9)
Assignee | ||
Comment 14•10 years ago
|
||
Updated Try run, with tests (and a few other patches, which might be to blame in the unlikely event of test-failures): https://tbpl.mozilla.org/?tree=Try&rev=02c0798aa406
Comment 15•10 years ago
|
||
Comment on attachment 8415550 [details] [diff] [review] fix v2 (now fixing the usage inside of MIX_IMPL macro, too) Review of attachment 8415550 [details] [diff] [review]: ----------------------------------------------------------------- ok, let's land this for the warnings, though please file a follow-up to rename the internal methods in StorageBaseStatementInternal, cause imo it's a convoluted foot gun like it is :)
Attachment #8415550 -
Flags: review?(mak77) → review+
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(mak77)
Assignee | ||
Comment 16•10 years ago
|
||
Thanks! I spun off bug 1004531 for the renaming/refactoring. Feel free to elaborate over there, if my summary didn't capture what you're getting at. :) Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/32caddb106d9
Assignee | ||
Updated•10 years ago
|
Attachment #8415653 -
Attachment mime type: text/x-c++src → text/plain
https://hg.mozilla.org/mozilla-central/rev/32caddb106d9
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in
before you can comment on or make changes to this bug.
Description
•