Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix namespace of move function in tests.cc #63

Merged
merged 1 commit into from
Feb 22, 2024

Conversation

Yvan-xy
Copy link
Contributor

@Yvan-xy Yvan-xy commented Feb 22, 2024

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

@max0x7ba
Copy link
Owner

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 Yvan-xy force-pushed the master branch 2 times, most recently from edb4fd0 to 94916ec Compare February 22, 2024 23:26
@Yvan-xy
Copy link
Contributor Author

Yvan-xy commented Feb 22, 2024

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

@max0x7ba
Copy link
Owner

max0x7ba commented Feb 22, 2024

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.

@max0x7ba max0x7ba merged commit 10ae35d into max0x7ba:master Feb 22, 2024
6 checks passed
@musicinmybrain
Copy link
Contributor

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. 😄

(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
Copy link
Owner

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
Copy link
Contributor

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
Copy link
Contributor Author

Yvan-xy commented Feb 23, 2024

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. 😄

(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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants