Closed Bug 1003902 Opened 6 years ago Closed 6 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)

All
Windows XP
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files, 2 obsolete files)

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.
hg archeology shows that this has been an issue ever since bug 507414 added these files (mozIStorageBaseStatement.idl & StorageBaseStatementInternal.h).
Depends on: 507414
Attached patch (Wrong patch file) (obsolete) — Splinter Review
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
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)
Attached patch fix v1 (obsolete) — Splinter Review
(er, attached wrong patch file before. here's the right one.)
Attachment #8415364 - Attachment is obsolete: true
Attachment #8415364 - Attachment description: fix v1 → (Wrong patch file)
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 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...
(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.)
Attachment #8415380 - Attachment is obsolete: true
Attachment #8415380 - Flags: review?(mak77)
Attachment #8415550 - Flags: review?(mak77)
(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)
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.
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.
}
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)
(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)
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 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+
Flags: needinfo?(mak77)
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
Attachment #8415653 - Attachment mime type: text/x-c++src → text/plain
https://hg.mozilla.org/mozilla-central/rev/32caddb106d9
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.