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 missing-braces issue for Rcomplex #135

Merged
merged 8 commits into from
Oct 31, 2024

Conversation

MichaelChirico
Copy link
Contributor

@MichaelChirico MichaelChirico commented Oct 30, 2024

Closes #134. This approach avoids the need for old-vs-new preprocessor branching.

@eddelbuettel
Copy link
Owner

No, not a fan of a preprocessor macro based approach, sorry.

Copy link
Owner

@eddelbuettel eddelbuettel left a comment

Choose a reason for hiding this comment

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

Rejecting as using macros is step backwards. An inline function with preprocessor step for clang > whateverthreshold may work.

src/period.cpp Outdated Show resolved Hide resolved
@MichaelChirico
Copy link
Contributor Author

Sure! Made it a routine instead, PTAL.

@MichaelChirico
Copy link
Contributor Author

preprocessor step for clang > whateverthreshold

I'm taking the approach of branching on R version because that's where the actual typedef for Rcomplex changes, a branch on the compiler would just be to avoid compiler warnings. If we think the warning is good+correct, the R version branching is better.

src/period.cpp Outdated Show resolved Hide resolved
ChangeLog Outdated Show resolved Hide resolved
Copy link
Owner

@eddelbuettel eddelbuettel left a comment

Choose a reason for hiding this comment

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

Micro nags. We're getting there.

Copy link
Owner

@eddelbuettel eddelbuettel left a comment

Choose a reason for hiding this comment

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

Yup, that's pretty much what I would have done and the check on the R version may actually be net better than what I had in my first attempt. We can live with this.

@eddelbuettel
Copy link
Owner

Can you please check why macOS is crying?

@MichaelChirico
Copy link
Contributor Author

Argh. Sorry. I can't tell why. Do they give you and GHA artifacts that would include 00install.out? I didn't see any but I might just lack permissions.

Don't spend more than a few minutes on this, I'll try and figure out another way if it's causing you headache.

@eddelbuettel
Copy link
Owner

Do they give you and GHA artifacts that would include 00install.out?

We have to add a block such as towards the end of the file ci.yaml:

      - name: View Logs
        run: .run.sh dump_logs
        if: failure()

Otherwise I can try mac-builder later but I presume you have the requisite tar.gz handy?

@MichaelChirico
Copy link
Contributor Author

The earlier Mac issue was probably due to my sloppy copy-pasting, I had

makeComplex(double *re, double *im)
// then later
makeComplex(re, im); // Should be &re, &im!

Obviated the issue by just making copies (which I think we need to do and IINM is what the original code was doing).

src/period.cpp Outdated Show resolved Hide resolved
src/period.cpp Outdated Show resolved Hide resolved
src/period.cpp Outdated Show resolved Hide resolved
src/period.cpp Outdated Show resolved Hide resolved
Co-authored-by: Michael Chirico <chiricom@google.com>
@eddelbuettel eddelbuettel merged commit 07f677a into eddelbuettel:master Oct 31, 2024
2 checks passed
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.

Flagging compiler issue
2 participants