I think I found a bug in the noCopyConstructor and noOperatorEq checks on types with:
- Multiple inheritance
- One or more of the inherited types is "unknown"
- One or more of the inherited types is known to be copyable
The behavior depends on the order of the inherited types: When the last type in the list is known to be copyable, the check "forgets" about the "unknown" type(s) before it.
This is illustrated by the test added by the 0001 patch attached: The first assert passes, while the second fails.
The 0002 patch is a minimal fix for this.
Note:
I think that this is a "minimal" fix. Looking at how the calling code handles unknown, it seems to me that, as it is currently, it is not useful to make the distinction between "there is definitively a non-copyable base" (the function returns true) and "there is an unknown base" (unknown is true) since the emission of the error is skipped in either cases.
I chose to keep the change minimal and avoid going down that rabbit hole right now, but it may be better to either:
- Completely drop the unknown parameter and simply return true as soon as a non-copyable or an unknown base is found.
- Keep the unknown parameter and use it to emit an inconclusive warning, changing the calling code to something like:if (!hasNonCopyableBase(scope, &unknown) || (unknown && (mSettings->certainty.isEnabled(Certainty::inconclusive)))) (or something equivalent and less ugly).
I am afraid I don't volounteer to go down the rabbit hole neither right now.. but your second suggestion sounds reasonable to me. and I do not think it's too ugly.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Hi,
I think I found a bug in the noCopyConstructor and noOperatorEq checks on types with:
- Multiple inheritance
- One or more of the inherited types is "unknown"
- One or more of the inherited types is known to be copyable
The behavior depends on the order of the inherited types: When the last type in the list is known to be copyable, the check "forgets" about the "unknown" type(s) before it.
This is illustrated by the test added by the 0001 patch attached: The first assert passes, while the second fails.
The 0002 patch is a minimal fix for this.
Note:
I think that this is a "minimal" fix. Looking at how the calling code handles
unknown
, it seems to me that, as it is currently, it is not useful to make the distinction between "there is definitively a non-copyable base" (the function returnstrue
) and "there is an unknown base" (unknown
istrue
) since the emission of the error is skipped in either cases.I chose to keep the change minimal and avoid going down that rabbit hole right now, but it may be better to either:
- Completely drop the
unknown
parameter and simply returntrue
as soon as a non-copyable or an unknown base is found.- Keep the
unknown
parameter and use it to emit an inconclusive warning, changing the calling code to something like:if (!hasNonCopyableBase(scope, &unknown) || (unknown && (mSettings->certainty.isEnabled(Certainty::inconclusive))))
(or something equivalent and less ugly).Thanks
sorry for late reply.
I wonder if you would like to create a github pull request with this update.
Even if it is not the perfect fix I guess it is still an improvement.
I am afraid I don't volounteer to go down the rabbit hole neither right now.. but your second suggestion sounds reasonable to me. and I do not think it's too ugly.
Thanks for your reply. I can create a PR, but I don't have time now. Hopefully next week.