Skip to content

cplusplus: apply a few clang-tidy suggested fixes#4760

Merged
kleisauke merged 4 commits intolibvips:masterfrom
kleisauke:cpp-clang-tidy
Dec 2, 2025
Merged

cplusplus: apply a few clang-tidy suggested fixes#4760
kleisauke merged 4 commits intolibvips:masterfrom
kleisauke:cpp-clang-tidy

Conversation

@kleisauke
Copy link
Copy Markdown
Member

See the individual commits for more details.

The abi-compliance-checker results are available at:
https://kleisauke.nl/compat_reports/vips-cpp/master_to_clang-tidy/compat_report.html

Comment on lines +58 to 62
const char *
what() const noexcept override
{
return _what.c_str();
return std::runtime_error::what();
}
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I considered removing what(), since it adds no functional value beyond what the base class (std::runtime_error) already provides. However, doing so appears to break ABI compat, see:
https://kleisauke.nl/compat_reports/vips-cpp/master_to_clang-tidy-rev2/compat_report.html
https://github.com/kleisauke/libvips/compare/cpp-clang-tidy..cpp-clang-tidy-rev2

@jcupitt
Copy link
Copy Markdown
Member

jcupitt commented Dec 2, 2025

Wow I'm amazed you can change the exception base class without breaking ABI compatibility. The C++ ABI rules are very mysterious.

Is it possible it could break with some C++ compilers and not others?

Comment thread cplusplus/include/vips/VImage8.h Outdated
@kleisauke
Copy link
Copy Markdown
Member Author

My understanding is that this is safe because std::runtime_error also stores a std::string internally. However, I need to verify this, since the change in class size could(?) break code that catches VError by value (catch (VError err)) rather than by reference (catch (const VError &err)).

@kleisauke
Copy link
Copy Markdown
Member Author

... let's be on the safe side, commit 943d1fc ensures ABI compatibility, I now see:
https://kleisauke.nl/compat_reports/vips-cpp/master_to_clang-tidy-abi/compat_report.html

Copy link
Copy Markdown
Member

@jcupitt jcupitt left a comment

Choose a reason for hiding this comment

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

Great!

@kleisauke kleisauke merged commit 5900859 into libvips:master Dec 2, 2025
7 checks passed
@kleisauke kleisauke deleted the cpp-clang-tidy branch December 2, 2025 14:39
@lovell
Copy link
Copy Markdown
Member

lovell commented Jan 22, 2026

I've been investigating error message corruption in v8.18.0 on Windows and I suspect this change might have exposed us to microsoft/STL#2114. The call to vips_error_buffer now takes place sooner at construction time rather than deferring to a later what() call. Perhaps we should use vips_error_buffer_copy and manage the lifetime within the exception?

@kleisauke
Copy link
Copy Markdown
Member Author

@lovell According to PR nodejs/node-addon-api#1059, this define appears to be the root cause:
https://github.com/nodejs/node/blob/v25.4.0/common.gypi#L485-L486

I'm not sure if we should support the _HAS_EXCEPTIONS=0 case. This changeset appears to resolve the issue in sharp:
lovell/sharp@0.35...kleisauke:sharp:temp-err-2
(i.e. the define also needs to be propagated to consumers of VError8.h)

@lovell
Copy link
Copy Markdown
Member

lovell commented Jan 22, 2026

@kleisauke Thanks, that explains it, I was so close! It looks like STL will be altering its behaviour to copy the message so we can leave this alone.

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