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

Fix IborCoupon and Money data races #1187

Merged
merged 9 commits into from
Sep 28, 2021
Merged

Conversation

pkovacs
Copy link
Contributor

@pkovacs pkovacs commented Sep 18, 2021

Here's a fix for #1146. I added both IborCoupon::Settings and Money::Settings as nested classes per @ralfkonrad's suggestion. I deprecated the original static methods -- which still work -- they simply use the new singletons. The test suite and example code changes are simply to use the new apis. The Money class unfortunately did not use static methods, but static vars instead (ouch), so I did the best that I could with that.

I tested locally with QL_USE_INDEXED_COUPON off and then on.

Phil

@pkovacs pkovacs changed the title Data races Fix IborCoupon and Money data races Sep 18, 2021
@pkovacs
Copy link
Contributor Author

pkovacs commented Sep 18, 2021

The fails are due to coveralls.io being down for maintenance.

@ralfkonrad
Copy link
Contributor

Hi Philip,

I am pretty sure that the approach is technical correct. It is more or less the same as I initially suggested in #665 for the IborCoupon.

However from the business perspective I think we should take a different approach for the IborCoupon: createAtParCoupons() and createIndexedCoupons() means that there are two different flavors of the IborCoupon.

IMHO this translate into an abstract IborCoupon class with the implementations IndexedIborCoupon and AtParIborCoupon.

The way to achieve this would be

  • deprecating the current IborCoupon constructor and the static methods,
  • solving the data races temporarily using static std::atomic<bool> usingAtParCoupons_; till the static members get removed,
  • making const Date& IborCoupon::fixingEndDate() const = 0; abstract as this is the main difference between the two flavors.
  • introducing enum class IborCoupon::Type { AtPar, Indexed }; and an abstract inspector IborCoupon::Type IborCoupon::type() const = 0; to identify the coupon type,
  • introducing a protected IborCoupon constructor,
  • implementing the two classes IndexedIborCoupon and AtParIborCoupon and
  • solving all the other issues when making IborCoupon abstract.

I think this solves the issues we currently have with the IborCoupon implementation and is more in line with OO principles. Also QL_USE_INDEXED_COUPON can be deprecated and later removed.

The problem is that IborCoupon is so prominent that these changes have a big impact on client code.

@lballabio, @pcaspers What do you think about this approach?

@ralfkonrad
Copy link
Contributor

ralfkonrad commented Sep 21, 2021

I forgot to mention:

The new constructor arguments of IndexedIborCoupon and AtParIborCoupon would be the same as the current IborCoupon one.

So the client code "only" has to change all its current constructor calls to the desired indexed or at par one.

@lballabio
Copy link
Owner

I agree with aiming at removing the flag in the long run. In the short term, though, we might merge this PR and do the rest of the work later. (I'll probably try to release 1.24 in October.)

For the long-term solution: we already have the machinery for pricing the same coupon in different ways. I'd keep just one IborCoupon class (as it is now) and use two different pricer classes instead.

@ralfkonrad
Copy link
Contributor

Okay, no problem.

@coveralls
Copy link

coveralls commented Sep 24, 2021

Coverage Status

Coverage decreased (-0.001%) to 71.11% when pulling 54e2ccc on pkovacs:data-races into 6f4126f on lballabio:master.

@lballabio lballabio linked an issue Sep 24, 2021 that may be closed by this pull request
ql/money.cpp Outdated Show resolved Hide resolved
@pcaspers
Copy link
Contributor

as for the long term solution for the Ibor coupon that Luigi mentions above, see #1189

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.

ibor coupon static members cause data races
5 participants