Fix and test for issue #4117

[rwgk]

Description

See #3861 and #4117.

Suggested changelog entry:

TODO

[rwgk]

@laramiel thanks for suggesting the Python part of the test!

(I wasn't getting very lucky looking under https://github.com/wassimj/Topologic)

[rwgk]

@henryiii there is a test here.

[rwgk]

@lalaland @laramiel It's interesting that the CI succeeded for #4273, it simply replaces intrinsic_t<T> with T.

@lalaland did you come to the idea that intrinsic_t is unnecessary by inspecting the calling code?

I almost think it will be best to drop this PR and reopen and merge #4273 (the only hiccup was the unused ctypes import). Although it might be useful to add in the Python part of the test from here, which has a Python override returning self (@laramiel's idea). I believe it is redundant for the purpose of exercising the exact core fix, but it is a different situation that we evidently don't cover otherwise, and is very little extra code.

@henryiii for awareness.

BTW: Amazing coincidence that 3 people worked on fixes in 2 PRs at the same time.

[EthanSteinberg]

@lalaland <https://github.com/Lalaland> did you come to the idea that

intrinsic_t is unnecessary by inspecting the calling code? I switched it to T just to revert to the old behavior in 2.9 Your version with remove_cv is more correct (and an improvement over 2.9, as opposed to just fixing the regression). I can quickly reopen #4273 with the needed changes (remove_cv and returning self).

…

On Sat, Oct 22, 2022, 12:23 AM Ralf W. Grosse-Kunstleve < ***@***.***> wrote: @lalaland <https://github.com/Lalaland> @laramiel <https://github.com/laramiel> It's interesting that the CI succeeded for #4273 <#4273>, it simply replaces intrinsic_t<T> with T. @lalaland <https://github.com/Lalaland> did you come to the idea that intrinsic_t is unnecessary by inspecting the calling code? I almost think it will be best to drop this PR and reopen and merge #4273 <#4273> (the only hiccup was the unused ctypes import). Although it might be useful to add in the Python part of the test from here, which has a Python override returning self ( @laramiel <https://github.com/laramiel>'s idea). I believe it is redundant for the purpose of exercising the exact core fix, but it is a different situation that we evidently don't cover otherwise, and is very little extra code. @henryiii <https://github.com/henryiii> for awareness. BTW: Amazing coincidence that 3 people worked on fixes in 2 PRs at the same time. — Reply to this email directly, view it on GitHub <#4274 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACTRWJ5Z633JHZPSANCH4DWEOJAPANCNFSM6AAAAAARLU5JRY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[EthanSteinberg]

Don't actually have permission to reopen PRs, so created #4275

[Skylion007]

Suggested change

	virtual ~Animal() {}
	virtual ~Animal() = default;

[rwgk]

Closing, superseded by #4275

[rwgk]

FWIW / future reference: I rebased and squashed my branch after #4275 was merged, JIC we want to resurrect the test later. I also verified that the test succeeds as expected.