Skip to content

Conversation

neheb
Copy link
Collaborator

@neheb neheb commented Aug 13, 2025

Avoids needing a definition in header.

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the TiffEncoder class to use std::unique_ptr for managing ExifData, eliminating the need for the ExifData definition in the header file.

  • Changes ExifData member from value type to std::unique_ptr
  • Removes the exif.hpp include from the header file
  • Updates all ExifData usage to use pointer dereference syntax

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/tiffvisitor_int.hpp Removes exif.hpp include and changes ExifData member to unique_ptr
src/tiffvisitor_int.cpp Updates constructor and all methods to use pointer dereference for ExifData access

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

@neheb
Copy link
Collaborator Author

neheb commented Aug 14, 2025

Incredible. Only clang-cl errors on what I think would be an error everywhere. Incredible.

@neheb
Copy link
Collaborator Author

neheb commented Aug 16, 2025

reran CI.

@kevinbackhouse
Copy link
Collaborator

What's the benefit of this change? Is it just so that you can delete #include "exif.hpp"? But it introduces a new risk that exifData_ could be null, doesn't it?

@neheb
Copy link
Collaborator Author

neheb commented Aug 22, 2025

It's never NULL.

I can't use a reference here as it must be a copy.

edit: looking at the code, these references probably need const/removal. Will post an alternate PR.

@neheb neheb force-pushed the t branch 2 times, most recently from c0e1f8b to 498c851 Compare September 2, 2025 02:26
Avoids needing a definition in header.

Signed-off-by: Rosen Penev <[email protected]>
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.

2 participants