Skip to content
This repository was archived by the owner on Mar 21, 2024. It is now read-only.

<atomics> Remove defaulted copy constructor from __cxx_atomic_lock_impl #310

Merged
merged 3 commits into from
Sep 14, 2022

Conversation

miscco
Copy link
Collaborator

@miscco miscco commented Sep 7, 2022

Defaulting the copy constructor inhibits the compiler defaulting the move constructor. Also it is not needed here at all.

Fixes: nvbug3785347

@miscco miscco requested a review from wmaxey September 7, 2022 18:18
Defaulting the copy constructor inhibits the compiler defaulting the move constructor.
Also it is not needed here at all.

Fixes: nvbug3785347
@jrhemstad
Copy link
Collaborator

Defaulting the copy constructor inhibits the compiler defaulting the move constructor.

What if we just default the move ctor also? I tend to subscribe to the "Explicit Rule of 0" , but it's not a hill I'd die on.

@miscco
Copy link
Collaborator Author

miscco commented Sep 7, 2022

We should then also default all other special member functions and personally I am not a fan of adding mental load for no benefit.

@wmaxey
Copy link
Member

wmaxey commented Sep 7, 2022

I agree with removing ctor if it's not necessary. I had added it for some reason... and am currently investigating its requirement.

I do have a test fix for MSVC I'll be pushing soon.

@miscco
Copy link
Collaborator Author

miscco commented Sep 8, 2022

My general stance is that there is code and there is fundamental c++ library code.

With "normal" application code or even some framework I am all in on being more expressive even if not needed.

With a library like the STL it is different, because there are so many nuances and special cases that are actually needed, that adding "unnecessary" code adds a lot of mental overhead.

Why is this assignment written differently?
Why do we need to default something here?

Often there is a situation, where doing something slightly different is necessary and I want that to stand out.

@wmaxey wmaxey added this to the 1.9.0 milestone Sep 13, 2022
Fixes test failure:

```
2022-09-13 01:22:39+00:00| 11 | #  error "CUDA atomics are only supported for sm_60 and up on *nix and sm_70 and up on Windows."
2022-09-13 01:22:39+00:00| |    ^~~~~
2022-09-13 01:22:39+00:00| # --error 0x1 --
2022-09-13 01:22:39+00:00| --
2022-09-13 01:22:39+00:00| 
2022-09-13 01:22:39+00:00| Compilation failed unexpectedly!
2022-09-13 01:22:39+00:00| ********************
2022-09-13 01:25:32+00:00| 
2022-09-13 01:25:32+00:00| 6 warning(s) in tests
2022-09-13 01:25:32+00:00| ********************
2022-09-13 01:25:32+00:00| Failed Tests (1):
2022-09-13 01:25:32+00:00| libcu++ :: std/atomics/atomics.types.generic/non_trivial.pass.cpp
2022-09-13 01:25:32+00:00| 
2022-09-13 01:25:32+00:00| 
2022-09-13 01:25:32+00:00| Testing Time: 177.83s
2022-09-13 01:25:32+00:00| Unsupported      :  215
2022-09-13 01:25:32+00:00| Passed           : 1000
2022-09-13 01:25:32+00:00| Expectedly Failed:   17
2022-09-13 01:25:32+00:00| Failed           :    1
```
@miscco
Copy link
Collaborator Author

miscco commented Sep 13, 2022

Thanks a lot @wmaxey

@wmaxey wmaxey merged commit c9018b9 into NVIDIA:main Sep 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants