google / or-tools

Google's Operations Research tools:
https://developers.google.com/optimization/
Apache License 2.0
10.97k stars 2.1k forks source link

compilation failure due to switch from fabs to std::abs #356

Closed jmarca closed 6 years ago

jmarca commented 7 years ago

Compiling today, I ran into a problem:

src/glop/basis_representation.cc: In member function ‘operations_research::glop::Fractional operations_research::glop::BasisFactorization::ComputeInverseOneNorm() const’:
src/glop/basis_representation.cc:555:51: error: call of overloaded ‘abs(__gnu_cxx::__alloc_traits<std::allocator<double> >::value_type&)’ is ambiguous
       column_norm += std::abs(right_hand_side[row]);
                                                   ^
In file included from src/base/logging.h:18:0,
                 from src/glop/basis_representation.h:18,
                 from src/glop/basis_representation.cc:15:
/usr/include/stdlib.h:735:12: note: candidate: int abs(int)
 extern int abs (int __x) __THROW __attribute__ ((__const__)) __wur;
            ^
In file included from /usr/include/c++/5.4.0/ext/string_conversions.h:41:0,
                 from /usr/include/c++/5.4.0/bits/basic_string.h:5249,
                 from /usr/include/c++/5.4.0/string:52,
                 from /usr/include/c++/5.4.0/bits/locale_classes.h:40,
                 from /usr/include/c++/5.4.0/bits/ios_base.h:41,
                 from /usr/include/c++/5.4.0/ios:42,
                 from /usr/include/c++/5.4.0/ostream:38,
                 from /usr/include/c++/5.4.0/iostream:39,
                 from src/base/logging.h:19,
                 from src/glop/basis_representation.h:18,
                 from src/glop/basis_representation.cc:15:
/usr/include/c++/5.4.0/cstdlib:166:3: note: candidate: long int std::abs(long int)
   abs(long __i) { return __builtin_labs(__i); }
   ^
/usr/include/c++/5.4.0/cstdlib:174:3: note: candidate: long long int std::abs(long long int)
   abs(long long __x) { return __builtin_llabs (__x); }
   ^

Seems to be caused by commit fe66d48cee746408f2093a083b073aa8d60cec4c

re-ran "make third_party" with no change in outcome. I'm currently cleaning that out and redoing again just in case, and will continue to try to track it down on my end.

using gcc (GCC) 5.4.0 on slackware linux 14.2 (current)

jmarca commented 7 years ago

Following some hints from similar issues on the internet, and looking at the above error message, I did the following patch to fix the issue:

diff --git a/src/base/logging.h b/src/base/logging.h
index 2fdd6f613..ad995a112 100644
--- a/src/base/logging.h
+++ b/src/base/logging.h
@@ -16,6 +16,7 @@

 #include <assert.h>
 #include <stdlib.h>
+#include <cmath>
 #include <iostream>  // NOLINT
 #include "base/commandlineflags.h"
 #include "base/integral_types.h"

It seems like bad practice to put #include <cmath> into a file called logging.h and have that fix compilation problems related to the use of absolute value. So there is probably a better fix. In any event, logging.h is called enough that it gets the include cmath in where it needs to go I guess.

lperron commented 7 years ago

Hi,

Can you just try by adding #include in the .cc file?

Laurent Perron | Operations Research | lperron@google.com | (33) 1 42 68 53 00

2017-03-28 22:25 GMT+02:00 James Marca notifications@github.com:

Following some hints from similar issues on the internet, and looking at the above error message, I did the following patch to fix the issue:

diff --git a/src/base/logging.h b/src/base/logging.h index 2fdd6f613..ad995a112 100644 --- a/src/base/logging.h +++ b/src/base/logging.h @@ -16,6 +16,7 @@

include

include

+#include

include // NOLINT

include "base/commandlineflags.h"

include "base/integral_types.h"

It seems like bad practice to put "#include " into a file called logging.h and have that fix compilation problems related to the use of absolute value. So there is probably a better fix. In any event, logging.h is called enough that it gets the include cmath in where it needs to go I guess.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/google/or-tools/issues/356#issuecomment-289893580, or mute the thread https://github.com/notifications/unsubscribe-auth/AKj17cFEYdgG-dSrWdj1mFCsf9uBBsveks5rqWzTgaJpZM4MsD-c .

jmarca commented 7 years ago

Okay. Did that. So far I've needed to add #include <cmath> into the following cc files:

Unstaged changes (4)
modified   src/glop/basis_representation.cc
modified   src/glop/lu_factorization.cc
modified   src/glop/markowitz.cc
modified   src/glop/update_row.cc

Each time I added the line, compilation moved on to the next file, I added that line, etc.

Now there is a new error:

src/sat/boolean_problem.cc: In function ‘void operations_research::sat::UseObjectiveForSatAssignmentPreference(const operations_research::LinearBooleanProblem&, operations_research::sat::SatSolver*)’:
src/sat/boolean_problem.cc:236:78: error: ‘fabs’ was not declared in this scope
                           fabs(static_cast<double>(objective.coefficients(i))));
                                                                              ^
src/sat/boolean_problem.cc:240:60: error: ‘fabs’ was not declared in this scope
         fabs(static_cast<double>(objective.coefficients(i))) / max_weight;
                                                            ^
make: *** [makefiles/Makefile.gen.mk:1711: objs/sat/boolean_problem.o] Error 1

Because the patch I ref'd above seemed to swap out "fabs" for "std::abs" I am going to do that and see if I can't move on some more.

(Please be aware, I'm a competent programmer but the last time I really used C++ was back in 1992, using Stroustroup's first book on the language. I'm remembering by doing here!)

jmarca commented 7 years ago

Okay, that worked. I created a branch on my clone:

https://github.com/google/or-tools/compare/master...jmarca:bug/compilation_failure

Shall I make a pull request? It is a pretty simple fix.

(I submitted a CLA form, but haven't yet received confirmation, so I'm not going to pull request until I get it)

Mizux commented 6 years ago

I think this is fixed by https://github.com/google/or-tools/commit/c36066c39cb79108293ee0bd83cfc4227a4eb380 which adds <cmath> in ortools/lp_data/lp_types.h