Disable trivially_constructible test for atomic on MSVC++.

[BillyONeal]

MSVC++ implements P0883 unconditionally, which changes the rules for std::atomic. It removes atomic's trivial constructor, and makes the default constructor value initialize the T.

Note that Mongo was not following the C++11 rules, because it used the atomic before calling atomic_init first. MSVC++ never implemented the C++11 rules and previously default initialized the T.

All versions of MSVC++ will provide constant initialization of the guarded value "current_instance". In old versions, atomic didn't implement P0883 due to bugs in the constexpr evaluator; in current versions the constexpr evaluator was fixed and atomic value initializes unconditionally. Therefore, this PR disables the check whenever MSVC++'s standard library is detected.

See microsoft/STL#661 for further discussion.

[acmorrow]

@BillyONeal - I find the following language in the documentation for std::atomic_init on cppreference:

This function is provided for compatibility with C. If the compatibility is not required, std::atomic may be initialized through their non-default constructors.

The atomic is being constructed with an argument, so I think we are invoking one of its non-default constructors. Or perhaps we have fallen into one of the traps that motivated P0883? Are we actually violating the rules? Could you clarify?

Regarding the removal of the static_assert, I think this can't be the right answer. It has been many years since I worked on this code (and it has changed since my original formulation). But the static assertion was there for a reason. If the rules have changed and that static_assert is no longer true, it suggests that an important assumption is no longer valid.

[BillyONeal]

The atomic is being constructed with an argument, so I think we are invoking one of its non-default constructors.

Good point. But if that's true, the trivial constructability static assert doesn't mean anything, since you're not engaging the trivial ctor even if it were trivial. So maybe the right fix is to delete it outright?

[BillyONeal]

If the rules have changed and that static_assert is no longer true, it suggests that an important assumption is no longer valid.

Maybe; I'm ill equipped to make global assumptions about the code in question.

[BillyONeal]

@acmorrow According to "blame" that static assert was added in db943d5 as a means of avoiding a dynamic initializer but the author didn't realize that the assertion they made is unrelated to what they actually wrote.

EDIT: And by "they" in this case turns out I meant "you" :)

[acmorrow]

@BillyONeal - Yup, I started this. As I noted, I don't work actively on this project right now, though others do, and it has been quite a few years since I wrote that. I will need to go do some digging to uncover the reasoning behind the code. I remember that we spent considerable time evaluating solutions and that we felt we were in the right with out decision, but of course we may have got it wrong!

[BillyONeal]

I will need to go do some digging to uncover the reasoning behind the code.

The comment on the commit indicates that it's intending to avoid a dynamic initializer (presumably to avoid the 'static initialization order fiasco') which would make sense given that it's asserting triviality. Unfortunately both I and the original commit didn't pay attention that the default ctor isn't being used here so old versions of VC would give you a dynamic initializer anyway.

[lidanger]

So should I remove this line in v3.5 or change decltype(current_instance) to v_noabi::instance *?

[acmorrow]

@BillyONeal - Sorry for the delay. I did some archaeology on this. The relevant background is in:

• CXX-890 Don't use objects requiring dynamic init/fini for instance::c… #513
• https://jira.mongodb.org/browse/CXX-890

It appears that the code has evolved since then, notably I think the ability to recreate the instance was removed as part of https://jira.mongodb.org/browse/CXX-1300, since the underlying C driver couldn't faithfully support it.

The idea in #513 was indeed to avoid a dynamic initializer, by ensuring that current_instance gets static/constant initialization.

The intention of the static_asserts was probably aimed as best as we knew how at ensuring that 1) we would get such initialization and 2) that if someone "improved" the code in such a way that violated that intention that they would be alerted.

Given that I spend a lot less time on the details of C++ these days, can you clarify for me a few things?

• Given that this is the C++11 driver, we must abide by C++11 rules. Does C++11 ensure constant initialization fo the declaration of current_instance as declared in this code?
• If later versions of the standard have changed the rules here, is there anything we need to do differently on a per-version basis?
• What are the strongest assertions we can write that will trip if someone were to alter this code in a way that broke the assumption of constant initialization for current_instance?

[BillyONeal]

Does C++11 ensure constant initialization fo the declaration of current_instance as declared in this code?

Yes, but lots of compilers (e.g. VS2019 before 16.6) were buggy about it.

If later versions of the standard have changed the rules here, is there anything we need to do differently on a per-version basis?

Since you're using the constructor that initializes the contained value, I don't believe so. If you were default initializing it you would have needed to call atomic_init under the C++11 rules (that AFAIK nobody implements).

What are the strongest assertions we can write that will trip if someone were to alter this code in a way that broke the assumption of constant initialization for current_instance?

I don't believe there are any assertions you can write. Note that the existing assertion does not actually assert anything about current_instance.

[acmorrow]

What are the strongest assertions we can write that will trip if someone were to alter this code in a way that broke the assumption of constant initialization for current_instance?

I don't believe there are any assertions you can write. Note that the existing assertion does not actually assert anything about current_instance.

@BillyONeal

Sure, but it asserts something about the type of current instance. However, that assertion is also irrelevant since we are using the value-taking constructor per prior discussion.

Given all this, and your other answers above, my recommendation would be to update this PR to unconditionally remove the trivially_constructible test on all platforms, and then I'll be happy to get this merged.

[BillyONeal]

Done.

[acmorrow]

@BillyONeal - LGTM, thank you for being patient while I sorted through the history. @kevinAlbs, I think this should get merged, but probably by one of the active C++ driver developers.

[samantharitter]

These changes LGTM, with P0883 that static_assert is not useful and I think it's a good idea to get rid of it. But, @BillyONeal can you explain in more detail why we expect constant initialization for current_instance?

[kevinAlbs]

LGTM. Thank you @BillyONeal for the contribution and explanation, and to @acm for the background. It seems reasonable to remove this assertion if we were not invoking the default constructor.

[BillyONeal]

@BillyONeal can you explain in more detail why we expect constant initialization for current_instance?

Because it's calling a constexpr constructor.

Note that on Visual Studio before 2019 version 16.6, it is not getting constant initialization.

[kevinAlbs]

Merging, thanks again @BillyONeal!