KEMBAR78
<atomics> Remove defaulted copy constructor from __cxx_atomic_lock_impl by miscco · Pull Request #310 · NVIDIA/libcudacxx · GitHub
Skip to content
This repository was archived by the owner on Mar 21, 2024. It is now read-only.

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