Skip to content

Change DmpAlg to use preallocated arrays #283

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

calewis
Copy link

@calewis calewis commented Aug 12, 2025

DmpAlg isn't called with nr_order_ > 3 so this change saves 9 news and 9 frees for each construction and destruction of the class.

@CLAassistant
Copy link

CLAassistant commented Aug 12, 2025

CLA assistant check
All committers have signed the CLA.

dcalc/DmpCeff.cc Outdated
p_ = new double[nr_order_];
fjac_ = new double*[nr_order_];
// How do you want to check this?
assert(nr_order_ <= max_nr_order_);
Copy link
Author

Choose a reason for hiding this comment

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

Not sure how OpenSTA usually checks things like this. Let me know and I'll update it to the preferred method.

Copy link
Collaborator

@jjcherry56 jjcherry56 Aug 13, 2025

Choose a reason for hiding this comment

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

Never with assert (tag search or grep for assert and you will see that yourself). You can answer your own question by reading the other code in the same file. As an exercise I want you to explain why the check is not necessary.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the feedback. I've changed the function to throw a DmpError if the order is greater than max_nr_order_. I've taken another look at the code, and I'm still having trouble seeing what makes the check unnecessary.

@jjcherry56
Copy link
Collaborator

If you want a project I would not stop there if I was working on this (I wrote this 25 years ago).
I would make dmp_cap_, dmp_pi_, dmp_zero_c2_ member objects instead of heap allocated.
The next (harder) step would be to move the arrays to the DmpCeff class so they are shared instead
of having 3 copies of them.

But this PR is simple enough to use as a first step.

DmpAlg isn't called with nr_order_ > 3 so this change saves 9 news and 9
frees for each construction and destruction of the class.
@jjcherry56
Copy link
Collaborator

Look at the DmpAlg constructor calls

@calewis
Copy link
Author

calewis commented Aug 14, 2025

I'm aware that DmpAlg is currently not called with any nr_order above 3. But If someone did change to call it with a larger number I think they'd appreciate the error message instead of the issues they'd get writing/reading past the end of their data.

@calewis
Copy link
Author

calewis commented Aug 18, 2025

@jjcherry56 is the check the only thing you'd liked changed? I'd prefer to keep it, but I'll remove it if that's your only objection.

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