Applied -Wconversion and Fixed the Warnings by ShikharJ · Pull Request #1147 · symengine/symengine

ShikharJ · 2016-12-09T13:42:24Z

@certik @isuruf Pardon me for bothering you with such frugal commit messages, but due to the sheer number of the issued warnings, it was difficult to keep track of fixed files. -Wconversion has been removed in the latest commit, just to test the changes. When the work is close to completion, it will be re-applied.
Relevant: #228

certik · 2016-12-09T15:42:34Z

Thanks for fixing the warnings!

isuruf · 2016-12-10T05:51:01Z

symengine/fields.cpp

    GaloisFieldDict f(*this);
    unsigned n = this->degree();
-    auto k = std::ceil(std::sqrt(n / 2));
+    auto k = static_cast<unsigned>(std::sqrt(n / 2) + 1);


Can you keep the std::ceil?

isuruf · 2016-12-10T05:51:38Z

symengine/fields.h

@@ -510,7 +510,7 @@ class GaloisFieldDict

    unsigned int size() const


This should be changed to size_t.

isuruf · 2016-12-10T05:52:22Z

symengine/fields.h

        if (dict_.empty())
            return 0;
-        return dict_.size() - 1;
+        return static_cast<unsigned>(dict_.size() - 1);


isuruf · 2016-12-10T05:52:41Z

symengine/polys/upolybase.h

@@ -307,7 +307,7 @@ class ODictWrapper

    unsigned int size() const


isuruf · 2016-12-10T05:54:31Z

benchmarks/lwbench.cpp

-    return std::chrono::duration_cast<std::chrono::nanoseconds>(t2 - t1).count()
+    return static_cast<double>(
+               std::chrono::duration_cast<std::chrono::nanoseconds>(t2 - t1)
+                   .count())


Is this change really necessary?

@isuruf -Wconversion catches the implicit type conversion happening here (since the above expression returns an unsigned int). Hence the change.

ShikharJ · 2016-12-10T11:11:27Z

@isuruf @certik I have tried my best to fix the maximum number of issued warnings. Still, about 3 warnings are issued in Teuchos library, one in symengine_casts.h and another at symengine_rcp.h, (a total of 5), for which I am unable to find a working fix. -Wconversion has been re-applied though.

isuruf · 2016-12-10T11:23:53Z

Can you post the 5 warnings?

ShikharJ · 2016-12-10T12:26:27Z

@isuruf

/home/shikhar/symengine/symengine/utilities/teuchos/Teuchos_RCPNode.cpp:309:32: warning: conversion to ‘int’ from ‘std::multimap<const void*, {anonymous}::RCPNodeInfo>::size_type {aka long unsigned int}’ may alter its value [-Wconversion]
   return rcp_node_list()->size();
                                ^

/home/shikhar/symengine/symengine/utilities/teuchos/Teuchos_TestForException.cpp:87:15: warning: conversion to ‘int’ from ‘std::__cxx11::basic_string<char>::size_type {aka long unsigned int}’ may alter its value [-Wconversion]
   break_on_me = errorMsg.length(); // Use errMsg to avoid compiler warning.
               ^

/home/shikhar/symengine/symengine/utilities/teuchos/Teuchos_stacktrace.cpp:383:41: warning: conversion to ‘int’ from ‘std::vector<long long unsigned int>::size_type {aka long unsigned int}’ may alter its value [-Wconversion]
     return this->stacktrace_buffer.size();
                                         ^

/home/shikhar/symengine/symengine/symengine_rcp.h:380:19: warning: conversion to ‘unsigned int’ from ‘long int’ may alter its value [-Wconversion]
     RCP<T> p = rcp(new T(std::forward<Args>(args)...));
                   ^

/home/shikhar/symengine/symengine/symengine_casts.h:36:12: warning: conversion to ‘unsigned int’ from ‘double’ may alter its value [-Wfloat-conversion]
     return f;
            ^

Edit: @isuruf @certik I think, I have finally found a fix to 4 of the above warnings (except the one raised in symengine_rcp.h). Please review and suggest any changes.
It also looks like more warnings are cropping up in Travis. I'll continue looking into them.

certik

I think in general, using static_cast to get rid of the warning is the wrong fix. The warning says that the conversion might alter its value. Well, so would static_cast, wouldn't it? I would even argue that using such a fix makes things worse --- before we at least got a warning, now it will silently fail, if there is a problem.

I think the right fix is to keep returning unsigned integer, so that one doesn't have to convert at all. There might be some exceptions, but I left some comments where I think static_cast is not the way to go.

certik · 2016-12-10T22:51:08Z

symengine/symengine_casts.h

 inline To implicit_cast(const From &f)
 {
-    return f;
+    return static_cast<To>(f);


I don't think that's the correct fix. I think this should be left as it was, to allow the compiler to check types.

certik · 2016-12-10T22:53:20Z

symengine/utilities/teuchos/Teuchos_stacktrace.cpp

  }
  int get_size() const {
-    return this->stacktrace_buffer.size();
+    return static_cast<unsigned>(this->stacktrace_buffer.size());


Here I think the get_size should return size_t argument.

certik · 2016-12-10T22:54:27Z

symengine/utilities/teuchos/Teuchos_stacktrace.cpp

  const size_t stacktrace_size = 0;
 #endif
-  return rcp(new StacktraceAddresses(stacktrace_array, stacktrace_size,
+  return rcp(new StacktraceAddresses(stacktrace_array, static_cast<unsigned>(stacktrace_size),


The argument of StacktraceAddresses should accept size_t instead.

certik · 2016-12-10T22:55:03Z

symengine/sparse_matrix.cpp

                              const vec_basic &x)
 {
-    unsigned nnz = x.size();
+    unsigned nnz = static_cast<unsigned>(x.size());


Use size_t instead of unsigned?

This is actually okay since the row indices stored in CSRMatrix are unsigned.

ShikharJ · 2016-12-11T06:54:40Z

@certik Indeed, static_cast is not the way to go. The point you mentioned (of silent failures) is one of the reasons why many C++ development authors explicitly suggest people to minimise casting. However, the reason I resorted to casting was firstly to signify places that needed improvements. Secondly, I had my doubts about whether a particular "up-gradation" (say changing the return type from int to long) or "down-gradation" (changing from size_t to unsigned) should be applied at a place or not. Instead of clarifying them one by one, I thought it was better to have them reviewed all at once, so that the changes can be applied where they are utmost necessary.

isuruf · 2016-12-12T13:08:21Z

See my changes

certik · 2016-12-12T15:04:56Z

It looks like primesieve has these conversion issues as well.

isuruf · 2017-03-29T16:43:49Z

@ShikharJ, ping

…ymengine#228.

This will avoid warnings in inlined functions in dependency headers

mpfr prec should fit in double

ShikharJ · 2017-03-30T11:18:08Z

symengine/series_flint.cpp

    while (i <= s.degree())
-        if (not s.get_coeff(i++).is_zero())
-            return i - 1;
+        if (not((s.get_coeff(i++)).is_zero()))


@isuruf This line is giving away this error:

error: conversion to ‘unsigned int’ from ‘long int’ may alter its value [-Werror=conversion]

even though is_zero() would return a bool. Any idea why is this cropping up?

Is .get_coeff expecting a long?

I didn't notice that it wasn't. I'll update this. BTW, can you also enable the auto-cancellation of Travis builds?

isuruf · 2017-03-30T12:44:04Z

symengine/polys/basic_conversions.h

            powr = div(one, genpow);
            if (is_a<const Integer>(*powr)) {
-                int i = down_cast<const Integer &>(*powr).as_int();
+                int i = static_cast<int>(


Don't use static_cast when you don't know whether it will fit or not

isuruf · 2017-03-30T12:44:39Z

symengine/polys/uintpoly_flint.h

    }

-    unsigned int size() const
+    int size() const


Can this be negative?

@isuruf I don't think it can. Initially, it was giving an error such as this:

symengine/symengine/polys/upolybase.h:377:17: error: overriding ‘int SymEngine::UPolyBase<Container, Poly>::size() const [with Container = SymEngine::fmpq_poly_wrapper; Poly = SymEngine::URatPolyFlint]’

It didn't make much sense to change the parent function. Hence it was changed here. What should be the approach here?

I don't understand the purpose of having two functions degree and size, but if it works, okay. cc @srajangarg

size() : if p == 0 : return 0 else : return degree() + 1

You yourself have made the change in the parent function it seems.

@srajangarg @isuruf
Seems like that. The change was made in this commit in upolybase.h. I've tried reverting this back, but then we'll have to make even more changes (currently, we just have to account for two size() functions, one in uintpoly_flint.h and other in uintpoly_piranha.h).

@srajangarg, yes I know, but what is the use of having the two functions?

Flint has both the functions, so I thought it was standard to provide both.

ShikharJ · 2017-03-30T15:50:56Z

@isuruf This is ready for a review.

isuruf · 2017-03-30T16:08:53Z

Looks good to me, @certik, can you review?

ShikharJ · 2017-04-08T16:41:24Z

Ping @certik.

ShikharJ · 2017-04-14T06:39:14Z

Ping @certik.

certik · 2017-04-14T17:00:19Z

The changes look good. Can we enable the -Wconversion on Travis and make it an error?

certik · 2017-04-14T17:00:29Z

Are benchmarks affected?

isuruf · 2017-04-14T17:08:01Z

symengine/symengine_casts.h

+{
+    SYMENGINE_ASSERT(f <= std::numeric_limits<To>::max());
+    SYMENGINE_ASSERT(f >= std::numeric_limits<To>::min());
+    return static_cast<To>(f);


@certik, in Release mode numeric_cast should have the same cost as static_cast (which is zero)

isuruf · 2017-04-14T17:09:10Z

symengine/CMakeLists.txt

 endif()
+
+if (CMAKE_CXX_COMPILER_ID MATCHES "GNU|Clang")
+    set_target_properties(symengine PROPERTIES COMPILE_FLAGS "-Wconversion -Wno-sign-conversion")


This warning is enabled by default and in travis-ci all warnings are converted to errors

isuruf · 2017-04-16T19:43:52Z

@ShikharJ, thanks for the PR

Applied -Wconversion and Fixed the Warnings

isuruf requested changes Dec 10, 2016

View reviewed changes

ShikharJ force-pushed the Issue228 branch from c430c56 to 6e1426a Compare December 10, 2016 10:56

ShikharJ force-pushed the Issue228 branch from 9249db3 to 7f4a356 Compare December 10, 2016 18:54

certik requested changes Dec 10, 2016

View reviewed changes

isuruf force-pushed the Issue228 branch from 7f4a356 to 69d27f0 Compare December 11, 2016 05:55

isuruf approved these changes Dec 11, 2016

View reviewed changes

ShikharJ and others added 16 commits March 30, 2017 15:41

Applied -Wconversion and Reduced Warnings from 1037 to 685. Part of s…

ceb88e5

…ymengine#228.

Applied Changes and Fixed Remaining Bugs. Part of symengine#228.

4ac9ccd

Fix warnings and use -Wconversion on symengine only

55e4257

Fix benchmarks warnings better

110851f

cast to unsigned for fields and series

fb42406

Fix multinomial_coefficient inputs

e627827

Fix finitediff.cpp

8d6ad81

Add numeric_cast

b8780ba

Use numeric_cast

20e6a02

Mark dependency include directories as SYSTEM

ebddfcf

This will avoid warnings in inlined functions in dependency headers

Fix long to double conversion warning

0501c55

mpfr prec should fit in double

Fix a warning about unused local typedefs

83df9e7

Fix signed conversion issues

a482c08

Add Integer::as_uint

648a933

Fix more signed conversion issues and boostmp conversions

0dae42a

Disable Wsign-conversion for now

e36e55c

Fix std::cerr not found error

102e281

ShikharJ force-pushed the Issue228 branch from fe056e5 to 4746ea5 Compare March 30, 2017 10:57

ShikharJ commented Mar 30, 2017

View reviewed changes

ShikharJ force-pushed the Issue228 branch from 4746ea5 to 13f1ea4 Compare March 30, 2017 11:50

isuruf reviewed Mar 30, 2017

View reviewed changes

ShikharJ force-pushed the Issue228 branch from 13f1ea4 to d691c34 Compare March 30, 2017 13:41

More Fixes

fdef0c7

ShikharJ force-pushed the Issue228 branch from d691c34 to fdef0c7 Compare March 30, 2017 15:26

ShikharJ changed the title ~~[WIP] Applied -Wconversion and Reduced the Warnings~~ Applied -Wconversion and Fixed the Warnings Mar 30, 2017

Merge branch 'master' into Issue228

ce5ad40

isuruf reviewed Apr 14, 2017

View reviewed changes

isuruf merged commit e0bf1f7 into symengine:master Apr 16, 2017

ShikharJ deleted the Issue228 branch April 17, 2017 06:20

isuruf added a commit to isuruf/symengine that referenced this pull request Aug 4, 2018

Merge pull request symengine#1147 from ShikharJ/Issue228

9ec21f1

Applied -Wconversion and Fixed the Warnings

		@@ -510,7 +510,7 @@ class GaloisFieldDict

		unsigned int size() const

		@@ -307,7 +307,7 @@ class ODictWrapper

		unsigned int size() const

Conversation

ShikharJ commented Dec 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

certik commented Dec 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShikharJ Dec 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShikharJ commented Dec 10, 2016

Uh oh!

isuruf commented Dec 10, 2016

Uh oh!

ShikharJ commented Dec 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

certik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShikharJ commented Dec 11, 2016

Uh oh!

isuruf commented Dec 12, 2016

Uh oh!

certik commented Dec 12, 2016

Uh oh!

isuruf commented Mar 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShikharJ Mar 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srajangarg Mar 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShikharJ commented Mar 30, 2017

Uh oh!

isuruf commented Mar 30, 2017

Uh oh!

ShikharJ commented Apr 8, 2017

Uh oh!

ShikharJ commented Dec 9, 2016 •

edited

Loading

ShikharJ Dec 10, 2016 •

edited

Loading

ShikharJ commented Dec 10, 2016 •

edited

Loading

ShikharJ Mar 30, 2017 •

edited

Loading

srajangarg Mar 30, 2017 •

edited

Loading