Skip to content

Support std::pair constructors natively - #1440

Merged
vgvassilev merged 3 commits into
vgvassilev:masterfrom
PetroZarytskyi:forward
Aug 1, 2025
Merged

Support std::pair constructors natively#1440
vgvassilev merged 3 commits into
vgvassilev:masterfrom
PetroZarytskyi:forward

Conversation

@PetroZarytskyi

Copy link
Copy Markdown
Collaborator

This PR attempts to add support for std::pair constructors without specifying their custom derivatives. There were 2 challenges:

  1. Support base initializers in derived class constructors when building constructor pullbacks. Also, add support for them in TBR.
  2. Add support for std::forward.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

Comment thread include/clad/Differentiator/CladUtils.h
Comment thread include/clad/Differentiator/STLBuiltins.h
Comment thread lib/Differentiator/CladUtils.cpp
Comment thread lib/Differentiator/TBRAnalyzer.cpp
@codecov

codecov Bot commented Jul 8, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Comment thread test/Gradient/Constructors.C Outdated
// CHECK-NEXT: argByValWrapper *_this = (argByValWrapper *)malloc(sizeof(argByValWrapper));
// CHECK-NEXT: new (static_cast<argByVal *>(_this)) argByVal(v);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

What do we lose if we call argByValWrapper *_this = new argByValWrapper(...)? The memory will be initialized with some values but we will overwrite them. That would not lead to significant performance regression I'd think...

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Well, some constructor calls have side effects and shouldn't be called twice. A real-world example would be move-constructors. If that's better, we can check if the constructor doesn't have any side effects by examining its parameter types or other factors. Also, I think the question is more about the logic in constructor pullbacks in general than about this change in particular.

@PetroZarytskyi
PetroZarytskyi force-pushed the forward branch 2 times, most recently from dfff217 to c3a3224 Compare July 21, 2025 12:45

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

Comment thread lib/Differentiator/ReverseModeVisitor.cpp Outdated
@PetroZarytskyi
PetroZarytskyi force-pushed the forward branch 3 times, most recently from 15839d2 to b4678f0 Compare July 21, 2025 20:51
@PetroZarytskyi
PetroZarytskyi marked this pull request as ready for review July 21, 2025 21:07
@PetroZarytskyi
PetroZarytskyi force-pushed the forward branch 2 times, most recently from dfd51a1 to 4bc52ea Compare July 29, 2025 10:06
Comment thread include/clad/Differentiator/Differentiator.h Outdated

@vgvassilev vgvassilev left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

LGTM!

@vgvassilev
vgvassilev merged commit 088d032 into vgvassilev:master Aug 1, 2025
87 checks passed
@PetroZarytskyi
PetroZarytskyi deleted the forward branch August 2, 2025 14:37
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