> On 29 Mar 2018, at 10:05, Christophe Fergeau <cferg...@redhat.com> wrote:
>
> On Wed, Mar 28, 2018 at 05:54:21PM +0200, Christophe de Dinechin wrote:
>>> With that said, I find the current "ODR" portion of the commit log
>>> misleading, it's easy to get the impression that after this change, we
>>> won't have any ODR problem anymore (this is what I initially thought!).
>>
>> Why do you find it misleading?
>>
>> The precise wording is "The current plugin version check violates the
>> C++ ODR”. It specifically talks about “current plugin version check”.
>> And there is indeed no ODR violation in the version check after that
>> change.
>>
>> Whether there are ODR violations elsewhere, and whether we accept such
>> violations purposefully on a case by case basis for other calls is a
>> judgement call. The mechanism makes it entirely possible to avoid ODR
>> violations completely (at the cost of unnecessarily restricting binary
>> compatibility).
>>
>> So I don’t see it as misleading or implying that it magically gets rid
>> of other ODR violations.
>
> As long as we don't change the ABI, the version check is as much
> an ODR violation as any other part of the code.
In the present code, we violate the ODR *before* being able to assess if that’s
an ODR violation we are OK with.
Imagine you check for a null pointer. If you write:
if (ptr)
return ptr->foo();
else
return 0;
that’s OK, because you test the pointer before using it. There is no undefined
behavior in C++. However, if instead you write:
int Foo::foo()
{
if (this)
…
else
return 0;
}
then there is a violation of C++ semantics. It may work with some compilers,
some other compilers will optimize away the ‘if (this)’ test (I believe clang
does).
Our current version check looks like the second example, i.e. the test is done
too late, so that at the point where we test, we violate C++ semantics and the
test does not yield the expected results. In the most severe case, the test in
today’s code would crash instead of detecting the version check mismatch.
> Yet it's singled out as an ODR violation that has to be fixed in the commit
> log.
It’s singled out because the binary-compatibility check should not itself rely
on the code being binary compatible :-) So it’s different in that way.
For later ODR violations, it’s OK if the code “trusts” our binary compatibility
analysis, whatever that analysis may be. There is no known automated way to
detect an ODR violation in the general case, otherwise, compilers would do it…
> By reading it, yes I kind of expect that we are no longer having similar
> violations anywhere in the code, otherwise we would fix them too.
As I stated in my previous reply, the mechanism makes it *possible* for us to
entirely avoid later ODRs. But thats’ a *policy* that you can enforce with the
mechanism. It’s a choice. The trade-off, as I also explained in my previous
reply, is that if you stick to the strictest ODR policy, then practically any
change in the Agent class will be marked as binary incompatible. That policy
would be fine with me, btw, but with g++, it’s probably a bit too paranoid, and
not what Frediano for example initially intended.
In any case, could you suggest a wording that you think would be less
“misleading”?
> Imo what really matters here is that we need this check to detect ABI
> changes. which are problematic from an ODR point of view, it's not the ODR
> violation by itself. But it really is just a matter of wording.
The existing initial ODR violation is problematic inasmuch as its primary
objective is to assess if potential ODR violations are acceptable (“binary
compatible”) or not (“binary incompatible”).
Anyway, a better wording is welcome, as long as it is not less precise or
correct than the current one. Since I do not fully understand what you find
misleading in it, I cannot offer a suggestion.
Also, let’s make sure we are not once more spiralling into fruitless
bikeshedding. If the patch is correct and fixes an actual bug, and if the
wording is not blatantly incorrect (i.e. after fixing the spelling mistakes
Frediano pointed out), we might want to get the fix in instead of arguing for
days about it.
Christophe “had successfully avoided wearing a C++ lawyer hat for nearly 18
years, and was not too unhappy about it"
>
> Christophe
_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
