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

gfan 0.7 #238

Closed
wants to merge 1 commit into from
Closed

gfan 0.7 #238

wants to merge 1 commit into from

Conversation

mahrud
Copy link
Member

@mahrud mahrud commented Aug 22, 2024

No description provided.

@mahrud mahrud force-pushed the gfan branch 2 times, most recently from 3a454ee to 947eb46 Compare August 22, 2024 10:21
Formula/gfan.rb Outdated
+ for(int i=0;i<v.size();i++)if((ret=v[i]))break;
if(ret<0)ret=-ret;
assert(ret!=0);
for(int i=0;i<v.size();i++)ret=gcdGFAN(ret,v[i]);
Copy link
Member

Choose a reason for hiding this comment

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

This patch contains the contents of M2/libraries/gfan/patch-0.6.2, right? Do we know why these changes are necessary? I've only needed the <cstdint> patch for the Debian gfan package to build.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, a few things have changed.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right that the patches seem excessive. I'll look into whether the exit(1)s were necessary to fix something in the package.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Those exit's aren't in the Debian/Ubuntu, Fedora, or Gentoo gfan packages.

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably the package should be updated to reflect this ...

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the point of the exit's. They all appear in something like this:

assert(0);
exit(1);

But assert(0) will always abort the program, so we never even reach the call to exit.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I think once the int128 issue is resolved I'll just remove the exits.

@mahrud mahrud force-pushed the gfan branch 3 times, most recently from d35ccbf to 0de9588 Compare August 23, 2024 12:56
Comment on lines +180 to +77
template <> struct MyMakeUnsigned<int>{typedef unsigned int type;};
template <> struct MyMakeUnsigned<long int>{typedef unsigned long int type;};
+ template <> struct MyMakeUnsigned<long long int>{typedef unsigned long long int type;};
template <> struct MyMakeUnsigned<__int128>{typedef unsigned __int128 type;};
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if this makes sense. It seems like support for __int128 is patchy and on some systems it's just long long int, at least that's my guess from errors like

src/gfanlib_circuittableint.h:617:57: error: invalid use of incomplete type 'struct gfan::MyMakeUnsigned<long long int>'
  617 |                 typename MyMakeUnsigned<longword>::type positiveResultBoundTimesD=extMul(negabs(s).v,boundA.v);
      |                                                         ^~~~~~~~~~~~~~~~~~~~~~~~~

It's a mystery to me why having both declarations makes the compiler happy, but I hope it's not introducing an overflow bug.

Copy link
Member

Choose a reason for hiding this comment

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

These new __int128's are giving me lots of problem in the Debian package, too -- gfan won't even build on 32-bit systems any more.

@mahrud
Copy link
Member Author

mahrud commented Aug 23, 2024

I can't reproduce the build errors at home. @mikestillman can you try building the new gfan on macos-14 and giving me the patch that works for you?

@mahrud
Copy link
Member Author

mahrud commented Oct 20, 2024

@d-torrance I think I figured how to get all these builds to finish at once. It may help with the Ubuntu builds.

@github-actions github-actions bot closed this Oct 20, 2024
@github-actions github-actions bot closed this in b28814b Oct 20, 2024
@github-actions github-actions bot deleted the gfan branch October 20, 2024 23:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants