-
Notifications
You must be signed in to change notification settings - Fork 238
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
flintxx tests fail to compile with --enable-cxx #753
Comments
You can see CXXFLAGS in Makefile presumably. Our C++ has a tendency to break C++ compilers. However, perhaps @tthsqe12 can see something wrong. |
Does it work if you pass -std=c11 in CXXFLAGS? |
Sure, I'll try.
How does one get a verbose log?
…On Sat, 30 May 2020, 13:02 wbhart, ***@***.***> wrote:
Does it work if you pass -std=c11 in CXXFLAGS?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#753 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJXYHFN3EQ4OUVI3B3MSZDRUDYU7ANCNFSM4NOSLEKA>
.
|
At the moment it looks like you have to edit the QUIET_CXX in Makefile
directly. I'll open a ticket to add a verbose version, but I won't try to
add it for this release.
I think if you just set QUIET_CXX in the Makefile to just CXX it will be
verbose.
Bill.
On Sat, 30 May 2020 at 16:18, Dima Pasechnik <notifications@github.com>
wrote:
… Sure, I'll try.
How does one get a verbose log?
On Sat, 30 May 2020, 13:02 wbhart, ***@***.***> wrote:
> Does it work if you pass -std=c11 in CXXFLAGS?
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <#753 (comment)>, or
> unsubscribe
> <
https://github.com/notifications/unsubscribe-auth/AAJXYHFN3EQ4OUVI3B3MSZDRUDYU7ANCNFSM4NOSLEKA
>
> .
>
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#753 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAB3HJUC7FWL654TYIXQ5I3RUEIS7ANCNFSM4NOSLEKA>
.
|
I think you have a special |
Please see #755 in relation with this. |
I take it this is fixed by your patch that I just merged? |
still testing - it does not help that Flint tends to re-build and re-test everything :P (and for the life of me I can't figure out how to test a subset of Flint - I'd probably need to modify some loop in Makefile to do this) |
If you just want to test a single Flint module it's: make check MOD=module_name Flint uses dependency tracking, so I don't know why it's rebuilding everything, unless that's the standard behaviour of make. |
No, #755 and #756 didn't help, it still fails (despite correct CXXFLAGS being put into Makefile by I also don't get verbose output for Note that it appears that one gets C compiler compilling C++ files (cf. #753 (comment)) - I don't see how correct CXXFLAGS may be divined with this setup. Once the default |
You are right on both counts. The makefile is ignoring verbose when building tests (it works fine for the library). And the C compiler is used to compile the flintxx tests. Both are bugs. I think we need an EXTRA_CXX_TESTS and have it dealt with separately to the ordinary tests. I'll have a go at fixing it on Tuesday (Monday is a public holiday). Then we'll have another rc. |
It turns out the C++ compiler is used to compile the flintxx tests. What was wrong was the quiet mode was printing CC instead of CXX, but then calling CXX. However, the C++ tests are all failing. I have literally no idea why this is. I can't make head nor tail of C++ error messages, and no -std=blah seems to fix it for me. It seems like the C++ language has changed in completely incompatible ways since our tests were written. This is why Flint is a C library and not a C++ library. C++ is not a proper language. Anyhow, I can't fix this. I'm hoping @tthsqe12 may be able to figure out how to fix all the broken C++ tests.
@tthsqe12 can you let me know if you have time to look into this? I will however look into the verbose mode, which is certainly not working as expected. |
Ok I will test this out asap. |
It took me really a lot of hours (Make is an absolutely terrible language), but I seem to have fixed the verbose build feature. I can't do another rc until we sort out the failing C++ tests. I don't know how long that will take. |
@tthsqe12 the C++ test fail on ordinary Ubuntu linux with ordinary g++, no need to go to something as exotic as OpenBSD or clang. |
I didn't know that your CI does not run a part of tests, namely the C++ ones. Actually, clang/clang++ is the standard compiler on macOS with its Xcode, nothing too exotic. |
here is the verbose error output (on linux with g++ 8.3.1)
|
Oh, I can reproduce the error. It is just a matter of finding the offending commit/code. |
It might have stopped working just because C++ compilers got updated in the past years, and what worked on g++4 in 2013 is not working on g++8 in 2020, as the targeted by default C++ standard got bumped up. |
Indeed. The offending "commit" was my upgrade from gcc5 to gcc7. |
Just like English :-) One needs to talk about C++x, with x in {98,11,14,17} (at least). And different C++ compilers have different defaults for x, i.e. without an explict |
I got an easy fix (works at least with clang++8) - testing now. diff --git a/flintxx/test/t-fmpz_polyxx.cpp b/flintxx/test/t-fmpz_polyxx.cpp
index 36e0c81a4..2d87f94ed 100644
--- a/flintxx/test/t-fmpz_polyxx.cpp
+++ b/flintxx/test/t-fmpz_polyxx.cpp
@@ -12,6 +12,7 @@
#include <iostream>
#include <sstream>
#include <string>
+#include <functional>
#include "fmpz_polyxx.h"
#include "nmod_polyxx.h"
@@ -320,13 +321,13 @@ test_functions()
f *= 2;
ulong d = 0;
ltupleref(r, s, d) = pseudo_divrem(g, f);
- tassert(r*f + s == g*pow(2, d));
+ tassert(r*f + s == g*pow(2, std::cref(d)));
r = 0; r = 0; d = 0;
ltupleref(r, s, d) = pseudo_divrem_basecase(g, f);
- tassert(r*f + s == g*pow(2, d));
+ tassert(r*f + s == g*pow(2, std::cref(d)));
r = 0; r = 0; d = 0;
ltupleref(r, s, d) = pseudo_divrem_divconquer(g, f);
- tassert(r*f + s == g*pow(2, d));
+ tassert(r*f + s == g*pow(2, std::cref(d)));
tassert(pseudo_div(g, f).get<0>() == r);
tassert(pseudo_rem(g, f).get<0>() == s); |
I decided to fix it a better way. See if everything thing in the new pull request is good for you. |
this would pin it to the range c++11 - c++20, but, well... |
not sure how it's |
Sorry. I am not familiar with |
The issue with - tassert(r*f + s == g*pow(2, d));
+ ulong ddd = 1.0;
+ tassert(r*f + s == g*ddd); compiles just fine, so one can multiply a flint poly with a double (don't know why, though) |
There are several issues here that I am not fully understanding. What a headache. |
in good old days there was no pow() template, so it was easier for the
compiler. Now there is a maze of templates in cmath headers to ensure
compatibly.
…On Wed, 3 Jun 2020, 15:16 tthsqe12, ***@***.***> wrote:
There are several issues here that I am not fully understanding. What a
headache.
The d is a ulong. The first question is: what becomes of pow(2,d)? It is
my understanding that both 2 and d are converted to double and the return
is a double.
So lets assume now that we are dealing with fmpz_poly * double. There is
no fmpz_poly_mul_d function so the double has to be converted back to a
ulong, slong, or fmpz. Who decides which? In order for the test code to
have been passing in the past, this had to have been being decided
somewhere ...
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#753 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJXYHDSCMY45AR2HOT4SEDRUZLMTANCNFSM4NOSLEKA>
.
|
Ok. Sounds like no one knows the return type of |
To me, the explicit conversion of a double that you added is not needed.
By the way, would it be better to use a, most probably already available,
conversion to a rational?
What happens is that, due to the lazy templates evaluation, something
goes wrong in the rather old-style flintxx/expression code.
By right, it has to be debugged and fixed, and not papered over by either
your or my patch.
…On Wed, 3 Jun 2020, 15:58 tthsqe12, ***@***.***> wrote:
Ok. Sounds like no one knows the return type of pow(int, ulong). I hope
adding 0.5 to it and converting it to an fmpz is an acceptable solution to
the test code without relying on any particular version of c++.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#753 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJXYHEM7O3ZMHGWD7OVX4TRUZQIBANCNFSM4NOSLEKA>
.
|
Ok. This is probably something for @wbhart to keep in mind for 2.7. |
I will close this now, as I believe the substance of it has been dealt with. Obviously we still lack a maintainer for the C++ wrapper. It is just being maintained so it keeps passing its tests and no more. Volunteers welcome. |
IMHO this needs a follow-up issue, as this fix is just papering the problem over. |
You are of course welcome to open a separate issue explaining precisely what it is you would like to see implemented. But the C++ wrapper hasn't been maintained in years for lack of volunteers who know C++ well enough. The version of Flint that was originally wrapped had about 120,000 lines of code and took 3 months of work, 9 hours a day to wrap in C++. Current Flint is nearly 500,000 lines of code. So unless there is one hell of a motivated contributor, I don't see a hell of a lot happening. |
I meant to say that it's an issue that C++ support in Flint is dated and
unmaintained, and opening an issue saying precisely this might be a prudent
thing to do.
…On Thu, 4 Jun 2020, 00:43 wbhart, ***@***.***> wrote:
You are of course welcome to open a separate issue explaining precisely
what it is you would like to see implemented. But the C++ wrapper hasn't
been maintained in years for lack of volunteers who know C++ well enough.
The version of Flint that was originally wrapped had about 120,000 lines
of code and took 3 months of work, 9 hours a day. Current Flint is nearly
500,000 lines of code. So unless there is one hell of a motivated
contributor, I don't see a hell of a lot happening.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#753 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJXYHAO75MX3J3766CB6ELRU3N37ANCNFSM4NOSLEKA>
.
|
I get
This is with
and
CC
,CXX
set to clang and clang++ (version 8.0.1) on OpenBSD 6.7.I guess it is a pure C++ issue - how does one see the actual C++ invocations ?
If e.g. CXXFLAGS are ignored then a wrong
-std=
is used.The text was updated successfully, but these errors were encountered: