Re: [boost] "peer reviewed" - Rights and responsibilities of maintainers

Subject: Re: [boost] "peer reviewed" - Rights and responsibilities of maintainers
From: Alexander Grund (alexander.grund_at_[hidden])
Date: 2018-10-18 07:03:22

>
> const char* impossible = "impossible";
> return impossible != 
> boost::serialization::extended_type_info_typeid<float>::get_const_instance().get_key();
>
> I memory serves me, get_key() will return the address of string of 
> characters inside the the address space of the DLL.  This address is 
> compared to the address of "impossible" which is an address inside the 
> address space of the calling mainline application.  I could not and 
> cannot understand what is being tested here. Honestly, when I see 
> this, I felt it unproductive to continue. Given that EVERYONE has 
> looked at this and no one has seen any problem with it, I wondering if 
> I'm missing something really obvious and I would be grateful if anyone 
> could explain this to me.

As Peter noted it is a dummy comparison, its only purpose is to 
instantiate 2 different singletons in 2 different shared libraries. #128 
does the same but in a more non-obvious way, but in a "valid use" of 
Boost.Serialization. #111 is the condensed version of this.

 > Hmmm - but the value of the comparison is used in the determination 
of the test results.  Very unintuitive.  Why not compare to nullptr (or 
0 or whatever) which is what get_key() expected to return since no 
entries have been added to the extended type info table.

In response to those concerns raised recently I changed the code and 
comments:

- use "impossible !=..." to make it clear that the key cannot have this 
value and hence the result is always true
 Â  - Thats why I haven't used NULL anymore (I DID use it, but still you 
complained, so I made it more explicit that I don't care about WHAT 
get_key returns as long as it is called)
 Â  - I have to admit, I wasn't aware what get_key is supposed to return 
which is why i return "0" from main if they are NOT "NULL" and the 
default "0" if they are. You objected this as it is non-intuitive that 
main has an implicit "return 0" and I changed this noticing that get_key 
returned "NULL" when I did. To save time trying to find out what 
`get_key` is supposed to return I added "impossible"
- add a comment for this: "// Make sure to return true."
- There already was a hint, that different types are required: "// Use a 
different(!) type than multi_shared1"
- The purpose is in written in the doc of the cpp file: "library simply 
using extended_type_info_typeid"
- All of this is even explained in DETAIL in the main test file 
(https://github.com/boostorg/serialization/blob/0e2a9aa95b3ec6f7dde1db7ddfa4ccfc3e314632/test/test_multi_shared_lib.cpp)
- It is also explained in the PR and the testcase that "This crashes on 
termination"

Given all this I don't understand what was left so unclear, that you 
expected a "useful" test result. The whole purpose of this is to test IF 
it runs AT ALL which it currently doesn't. If any description is unclear 
and caused the confusion, simply tell me and I can amend it. That would 
be constructive and much appreciated.

 > There is also an interesting issue of using float and int as type 
arguments.  This is never been done before since no user of the 
serialization library would do this.  It's an interesting new variable 
in the mix and offhand I don't really know if there are any consequences 
to this.

Again: They are just used to instantiate the singletons whose 
destructions cause the crash. The same can be achieved with custom 
classes (see #127) but that makes the code more convoluted than required 
for a MWE. If there is an inherent failure in the lib why primitive 
types cannot be used, then I didn't found it. I don't see a difference 
when passing `int` as opposed to `Foo` from `struct Foo{};` and #127 
seems to support this. If this is what bothers you, please comment on 
#111 what types I shall pass instead (e.g. Foo and Bar both being simple 
structs), but I didn't deem it necessary.