8000 atf-check.cpp: remove unneeded copying into vector by rilysh · Pull Request #89 · freebsd/atf · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

atf-check.cpp: remove unneeded copying into vector #89

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 26, 2024
Merged

atf-check.cpp: remove unneeded copying into vector #89

merged 1 commit into from
Dec 26, 2024

Conversation

rilysh
Copy link
Contributor
@rilysh rilysh commented Dec 26, 2024

atf::fs::path has a str() method, and it returns a std::string constructor. I found this odd that a vector is used here to copy std::string contents. As std::string is a typedef of std::basic_string, this additional copy is not needed.

Also, remove calling the str() method on error.
Forgot to mention in commit, but end() + 1 is undefined behavior.

@rilysh
atf-check.cpp: remove unneeded copying into vector
25a33b6 
atf::fs::path has a str() method, and it returns a std::string
constructor. I found this odd that a vector is used here to copy
std::string contents. As std::string is a typedef of
std::basic_string<char>, this additional copy is not needed.

Also, remove calling the str() method on error.

Signed-off-by: rilysh <nightquick@proton.me>
ngie-eign
ngie-eign approved these changes Dec 26, 2024
View reviewed changes
Copy link
Contributor
@ngie-eign ngie-eign left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I thought the std::vector use was kind of unnecessary to begin with, but it’s probably an artifact of the original code. This is much cleaner than what I introduced yesterday.

@ngie-eign ngie-eign merged commit 7251187 into freebsd:master Dec 26, 2024
@ngie-eign ngie-eign mentioned this pull request Dec 26, 2024
Merged
@ngie-eign
Copy link
Contributor

This unfortunately resulted in a compiler error, so I needed to revert the change:

atf-sh/atf-check.cpp:122:26: error: cannot initialize a parameter of type 'char *' with an rvalue of type 'const value_type *' (aka 'const char *')
  122 |         m_fd = ::mkstemp(file_s.data());
      |                          ^~~~~~~~~~~~~
/Library/Developer/CommandLineTools/SDKs/MacOSX15.sdk/usr/include/_stdlib.h:213:20: note: passing argument to parameter here
  213 | int      mkstemp(char *);
      |                        ^
1 error generated.
make[1]: *** [atf-sh/atf_check-atf-check.o] Error 1
make[1]: *** Waiting for unfinished jobs....

We really need to get CI/CD setup for this repo...

@ngie-eign ngie-eign mentioned this pull request Dec 26, 2024
Merged
@ngie-eign
Copy link
Contributor

Proposed the change be reintroduced in #91 .

@rilysh
Copy link
Contributor Author
rilysh commented Dec 27, 2024 •
edited
Loading

This unfortunately resulted in a compiler error, so I needed to revert the change:

atf-sh/atf-check.cpp:122:26: error: cannot initialize a parameter of type 'char *' with an rvalue of type 'const value_type *' (aka 'const char *')
  122 |         m_fd = ::mkstemp(file_s.data());
      |                          ^~~~~~~~~~~~~
/Library/Developer/CommandLineTools/SDKs/MacOSX15.sdk/usr/include/_stdlib.h:213:20: note: passing argument to parameter here
  213 | int      mkstemp(char *);
      |                        ^
1 error generated.
make[1]: *** [atf-sh/atf_check-atf-check.o] Error 1
make[1]: *** Waiting for unfinished jobs....

We really need to get CI/CD setup for this repo...

Since C++17, data() has an overload of non-const, so it seems like an issue with the version of C++ that Apple Clang is defaulted to.
I've compiled with this patch on FreeBSD, and there isn't a problem. GCC and Clang on FreeBSD and Linux is defaulted to C++17.

Either way, instead of accessing via [] operator, just static_cast it to char * or set -std=c++17 (with no changes).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ngie-eign ngie-eign ngie-eign approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rilysh @ngie-eign
0