Added Function for Issue #1088 (Second Feature) by ShikharJ · Pull Request #1114 · symengine/symengine

ShikharJ · 2016-11-02T18:58:18Z

Added a function to call to indicate whether some components (namely flint, arb, mpc and mpfr) are installed or not. No tests are added (to prevent travis build failures). However the following links may be referred to for cross-checking the functionality:
(Build #9 For Rejecting Non MPFR builds)
https://travis-ci.org/ShikharJ/symengine/builds/172672603

(Build #8 For Rejecting MPFR builds)
https://travis-ci.org/ShikharJ/symengine/builds/172666980
The above tests have been conducted by patching the following lines of code:
char *s = "mpfr";
(#9)SYMENGINE_C_ASSERT(symengine_have_component(s));
(#8)SYMENGINE_C_ASSERT(!symengine_have_component(s));

certik · 2016-11-02T19:29:39Z

symengine/cwrapper.cpp

+        }
+    }
+    return 0;
+}


Instead of comparing strings like this, I think we should just introduce some enum constants and compare integers.

I'll make the changes. Thanks!

Enums are better when you have access to them via headers. This function is needed for libraries using only the shared library like SymEngine.jl which doesn't look at the headers. In that case strings are better.
When you have access to the headers, you don't need this function at all. You can just use, HAVE_SYMENGINE_MPFR to check whether it is installed.

isuruf

Building up two arrays and comparing is a waste and complex. Added a suggestion for a simple fix below.

isuruf · 2016-11-03T03:11:00Z

symengine/cwrapper.cpp

+{
+    bool t[4] = {};
+#ifdef HAVE_SYMENGINE_MPFR
+    t[0] = true;


You can do if (std::strcmp("mpfr", c)) return 1;

ShikharJ · 2016-11-03T08:36:35Z

@isuruf A review of the changes?

isuruf · 2016-11-03T08:46:43Z

@ShikharJ, can you add some tests?

isuruf · 2016-11-03T09:27:36Z

symengine/tests/cwrapper/test_cwrapper.c


+    // Rejecting non mpfr builds
+    s = "mpfr";
+    SYMENGINE_C_ASSERT(symengine_have_component(s));


As mentioned in the comment, this will fail when mpfr is not found. Use HAVE_SYMENGINE_MPFR and similar macros to test.

So I need to add another test rejecting mpfr and related builds?

Something like this,

#ifdef HAVE_SYMENGINE_MPFR SYMENGINE_C_ASSERT(symengine_have_component("mpfr") != 0); #else SYMENGINE_C_ASSERT(symengine_have_component("mpfr") == 0); #endif

ShikharJ · 2016-11-03T12:32:33Z

@isuruf Any further changes?

isuruf · 2016-11-03T14:23:43Z

symengine/tests/cwrapper/test_cwrapper.c

+    SYMENGINE_C_ASSERT(symengine_have_component(s));
+#else
+    SYMENGINE_C_ASSERT(!symengine_have_component(s));
+#endif


Can you add tests for the other components as well to increase coverage?

isuruf · 2016-11-03T14:25:03Z

symengine/cwrapper.h

 //! Frees the string s
 void basic_str_free(char *s);

+//! Returns 1 if a specific component is installed, 0 if not.


//! component can be "mpfr", "mpc", "flint" or "arb"

isuruf · 2016-11-03T14:26:10Z

symengine/cwrapper.cpp

+#ifdef HAVE_SYMENGINE_ARB
+    if (std::strcmp("arb", c) == 0)
+        return 1;
+#endif


If you want to add all the components, here is a list, https://github.com/symengine/symengine/blob/master/symengine/symengine_config.h.in#L13-L41

Thanks! I'll send in another commit with the changes.

isuruf · 2016-11-03T16:01:14Z

@certik, can you also review?

certik · 2016-11-03T17:11:48Z

I don't think we should be comparing strings like that. I think we should use C enums, like this:

symengine_have_component(SYMENGINE_FLINT)

where SYMENGINE_FLINT is an enum, declared in the C header file. Then inside, you change this:

#ifdef HAVE_SYMENGINE_MPC
    if (std::strcmp("mpc", c) == 0)
        return 1;
#endif

to this:

#ifdef HAVE_SYMENGINE_MPC
    if (c == SYMENGINE_MPC)
        return 1;
#endif

P.S. It's a good habit in C and C++ to always put braces {} for the if statement's branches, even if it's just a one liner like this.

isuruf · 2016-11-03T17:13:38Z

@certik, see my comment here, #1114 (comment)

isuruf · 2016-11-03T17:34:48Z

Travis is having a heavy load for OS X. Jobs will eventually run to completion. https://www.traviscistatus.com

ShikharJ · 2016-11-10T15:50:19Z

@certik Please take a look at the above comments and suggest any further changes, if necessary.

certik · 2016-11-10T15:54:35Z

Hi @ShikharJ, my objection is written in this comment: #1114 (comment), I don't think we should be using strings, just compare enums, see that comment for details.

ShikharJ · 2016-11-10T16:00:24Z

@certik Umm, I was referring to that same thread, but @isuruf 's comment below yours. To quote:
"Enums are better when you have access to them via headers. This function is needed for libraries using only the shared library like SymEngine.jl which doesn't look at the headers. In that case strings are better.
When you have access to the headers, you don't need this function at all. You can just use, HAVE_SYMENGINE_MPFR to check whether it is installed."

certik · 2016-11-10T17:30:32Z

@ShikharJ I see. In that case, can you add that explanation + motivation as a comment into the header file? So that it's clear when this function should be used, and when HAVE_SYMENGINE_MPFR should be used directly.

ShikharJ · 2016-11-13T06:15:40Z

@certik A final review?

isuruf · 2016-11-13T09:39:50Z

Thanks

Added Function for Issue symengine#1088 (Second Feature)

e39340d

certik reviewed Nov 2, 2016

View reviewed changes

isuruf reviewed Nov 3, 2016

View reviewed changes

Changes Made for Comparison

dca91f5

Added Test

db61afa

isuruf reviewed Nov 3, 2016

View reviewed changes

Updated Test

e48dc84

isuruf reviewed Nov 3, 2016

View reviewed changes

Added Exhaustive Tests and Full List of Components

e1c96cd

isuruf approved these changes Nov 3, 2016

View reviewed changes

Added Explanatory Comments

b1726d5

ShikharJ closed this Nov 11, 2016

ShikharJ reopened this Nov 11, 2016

ShikharJ closed this Nov 12, 2016

ShikharJ reopened this Nov 12, 2016

ShikharJ closed this Nov 13, 2016

ShikharJ reopened this Nov 13, 2016

isuruf merged commit 46b7285 into symengine:master Nov 13, 2016

ShikharJ deleted the Issue1088 branch November 13, 2016 10:54

isuruf mentioned this pull request Nov 26, 2016

Cwrapper improvements #1088

Closed

3 tasks

Conversation

ShikharJ commented Nov 2, 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!

isuruf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShikharJ commented Nov 3, 2016

Uh oh!

isuruf commented Nov 3, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShikharJ Nov 3, 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!

isuruf Nov 3, 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 Nov 3, 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!

isuruf commented Nov 3, 2016

Uh oh!

certik commented Nov 3, 2016

Uh oh!

isuruf commented Nov 3, 2016

Uh oh!

isuruf commented Nov 3, 2016

Uh oh!

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

Uh oh!

Uh oh!

certik commented Nov 10, 2016

Uh oh!

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

Uh oh!

Uh oh!

certik commented Nov 10, 2016

Uh oh!

ShikharJ commented Nov 13, 2016

Uh oh!

isuruf commented Nov 13, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ShikharJ Nov 3, 2016 •

edited

Loading

isuruf Nov 3, 2016 •

edited

Loading

ShikharJ commented Nov 10, 2016 •

edited

Loading

ShikharJ commented Nov 10, 2016 •

edited

Loading