max0x7ba / atomic_queue

C++ lockless queue.
MIT License
1.47k stars 176 forks source link

fix namespace of `move` function in tests.cc #63

Closed Yvan-xy closed 6 months ago

Yvan-xy commented 6 months ago

Failed to compiler the tests binary on ubuntu 22.04 by using clang-17. Because of the missing std::

max0x7ba commented 6 months ago

Failed to compiler the tests binary on ubuntu 22.04 by using clang-17. Because of the missing std::

I was under impression that unqualified move should use ADL and find std::move for its std::unique_ptr argument. And that worked flawlessly in C++14 mode. Am I missing something?

Your change to use qualified std::move disables ADL and that's something rather undesirable most of the time.

The correct fix would be using std::move; statement in that function or file. It is the best practice precisely because it doesn't disable ADL.

Don't you think so?

Yvan-xy commented 6 months ago

Thanks for you feedback. I've already updated the patch. :)

max0x7ba commented 6 months ago

I am still concerned with your change messing with my comment delimiter line for no good reason whatsoever. But I can stomach that 😂

Thank you for your contribution.

musicinmybrain commented 6 months ago

I am still concerned with your change messing with my comment delimiter line for no good reason whatsoever. But I can stomach that 😂

If it helps, tests.cc did not have a newline at the end of the file before this PR, and the commit added one. Most tools can handle the missing terminal newline, but the POSIX definition of a text file does require a newline at the end of each line, so the submitter’s editor (vim, perhaps?) probably added it automatically.

Now, I probably would have either omitted that change or at least split it into a separate commit to make it clear what was happening, but at least the submitter of this PR wasn’t just meddling with comment lines for fun. :smile:

(The other text files in this repository without terminal newlines are CMakeLists.txt, .github/workflows/cmake-gcc-clang.yml, include/CMakeLists.txt, and src/CMakeLists.txt.)

max0x7ba commented 6 months ago

I am still concerned with your change messing with my comment delimiter line for no good reason whatsoever. But I can stomach that 😂

If it helps, tests.cc did not have a newline at the end of the file before this PR, and the commit added one. Most tools can handle the missing terminal newline, but the POSIX definition of a text file does require a newline at the end of each line, so the submitter’s editor (vim, perhaps?) probably added it automatically.

You are quite right, Ben. I use emacs and am a bit perplexed how this file ended up without the trailing newline symbol in the first place. My mistake, I concede.

Now, I probably would have either omitted that change or at least split it into a separate commit to make it clear what was happening, but at least the submitter of this PR wasn’t just meddling with comment lines for fun. 😄

(The other text files in this repository without terminal newlines are CMakeLists.txt, .github/workflows/cmake-gcc-clang.yml, include/CMakeLists.txt, and src/CMakeLists.txt.)

You have quite a keen eye for terminal newlines, don't you? ;) But that is someone else's honest work and I'd rather not impose my arbitrary standards. And while Unix philosophy celebrates files, CMake philosophy celebrates strings.

musicinmybrain commented 6 months ago

You have quite a keen eye for terminal newlines, don't you? ;)

As a distribution packager, sometimes they matter to me! They don’t matter to me here, since the tools that consume these files are happy and the files are not ones that a distribution package would ship anyway.

Yvan-xy commented 6 months ago

I am still concerned with your change messing with my comment delimiter line for no good reason whatsoever. But I can stomach that 😂

If it helps, tests.cc did not have a newline at the end of the file before this PR, and the commit added one. Most tools can handle the missing terminal newline, but the POSIX definition of a text file does require a newline at the end of each line, so the submitter’s editor (vim, perhaps?) probably added it automatically.

Now, I probably would have either omitted that change or at least split it into a separate commit to make it clear what was happening, but at least the submitter of this PR wasn’t just meddling with comment lines for fun. :smile:

(The other text files in this repository without terminal newlines are CMakeLists.txt, .github/workflows/cmake-gcc-clang.yml, include/CMakeLists.txt, and src/CMakeLists.txt.)

Yes, I am using vim. I suppose that's the reason.