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

Simplify Null<T> implementation #2082

Merged
merged 3 commits into from
Oct 3, 2024
Merged

Simplify Null<T> implementation #2082

merged 3 commits into from
Oct 3, 2024

Conversation

lballabio
Copy link
Owner

  • reimplemented Null<T> (both function and class) with constexpr if, which is simpler to read and also more flexible since we can now define a reasonable default for non-numeric types and thus remove the specializations we used to need;
  • Array x = Null<Array>() is probably more readable as Array x = {};
  • null dates were sometimes written as Null<Date>() and sometimes as Date(), I settled on the latter throughout.

@lballabio lballabio added this to the Release 1.36 milestone Oct 3, 2024
@lballabio
Copy link
Owner Author

@pcaspers, may you check (not necessarily before this is merged or before release, no hurry) if this new implementation fixes #1462? If that's the case I would deprecate the configuration switch between function and class implementation. Thanks!

@coveralls
Copy link

Coverage Status

coverage: 72.676% (+0.003%) from 72.673%
when pulling ef12f99 on simplify-null
into 460a948 on master.

@lballabio lballabio merged commit 8bd5703 into master Oct 3, 2024
87 checks passed
@lballabio lballabio deleted the simplify-null branch October 3, 2024 12:39
@pcaspers
Copy link
Contributor

pcaspers commented Oct 3, 2024

I'll try, I have to ask our windows devs if they still have the particular msvc version in use! If you don't hear back on this, please assume there is no problem :-)

@auto-differentiation-dev
Copy link
Contributor

Looks like this caused some issue in Windows with C++17 for us though: https://github.com/auto-differentiation/QuantLib-Risks-Cpp/actions/runs/11174316341

We'll look into it shortly, might be something small / fixable in our code. Curious that it's Windows-only though, and works fine with Windows/C++20.

@lballabio
Copy link
Owner Author

Ok, thanks.

@lballabio
Copy link
Owner Author

Question—to what does std::is_floating_point_v<Real> evaluate in your setup? And what about std::is_floating_point<Real>::value, which should be the same?

@auto-differentiation-dev
Copy link
Contributor

We provide a standard compatibility header in XAD, which specialises those traits so that the XAD active types behave like the standard floating point types. This allows them to be used seamlessly with the standard library and eases porting existing codebases like QuantLib. We test this on all possible compilers we support to make sure it works smoothly.

The problem with std::is_floating_point_v<T> in MSVC is that its definition is not based on std::is_floating_point<T>, but it uses some internal MS helpers, so it needs explicit handling. But we believe we've fixed that now in this PR - we'll know shortly once this workflow run completes ok.

@lballabio
Copy link
Owner Author

Great, thanks.

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.

4 participants