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

random cleanups #3169

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

random cleanups #3169

wants to merge 15 commits into from

Conversation

neheb
Copy link
Collaborator

@neheb neheb commented Feb 17, 2025

No description provided.

@neheb neheb force-pushed the 4 branch 6 times, most recently from 1c1dd86 to ebd140d Compare February 18, 2025 06:00
@@ -5033,8 +5033,8 @@ const XmpPropertyInfo* XmpProperties::propertyInfo(const XmpKey& key) {
std::string property = key.tagName();
// If property is a path for a nested property, determines the innermost element
if (auto i = property.find_last_of('/'); i != std::string::npos) {
for (; i != std::string::npos && !isalpha(property.at(i)); ++i) {
}
while (!std::isalpha(property.at(i)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should probably check for \0. I agree that the previous i != std::string::npos check was wrong.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

at() here is probably wrong too.

Comment on lines +136 to +138
if (!value_.empty()) {
std::copy(value_.begin(), value_.end() - 1, std::ostream_iterator<int>(os, " "));
os << static_cast<int>(value_.back());
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems a weird way to write the code. Is it a standard idiom that I'm not familiar with? If so, please could you add a comment with a hyperlink to the relevant wikipedia page or similar?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think it's standard. The goal way to avoid a manual loop.

neheb added 13 commits February 21, 2025 13:03
Signed-off-by: Rosen Penev <rosenp@gmail.com>
Signed-off-by: Rosen Penev <rosenp@gmail.com>
Signed-off-by: Rosen Penev <rosenp@gmail.com>
Signed-off-by: Rosen Penev <rosenp@gmail.com>
Signed-off-by: Rosen Penev <rosenp@gmail.com>
Signed-off-by: Rosen Penev <rosenp@gmail.com>
Signed-off-by: Rosen Penev <rosenp@gmail.com>
Signed-off-by: Rosen Penev <rosenp@gmail.com>
Signed-off-by: Rosen Penev <rosenp@gmail.com>
These are no longer needed with modern compilers.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
simpler

Signed-off-by: Rosen Penev <rosenp@gmail.com>
Signed-off-by: Rosen Penev <rosenp@gmail.com>
Signed-off-by: Rosen Penev <rosenp@gmail.com>
std::swap was added here to avoid a use after move warning. But in
reality, the second usage of object is wrong and should be changed to
nullptr.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
@neheb
Copy link
Collaborator Author

neheb commented Feb 21, 2025

Rebased.

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