Fix string support

[ix-dcourtois]

Hi,

The first commit is a small fix to the CMake script to allow using a custom version of LLVM. The problem was that the script was allowing to set a LLVM_DIR option, but then it didn't use the configured paths to look for the llvm-config tool. So I had an old LLVM lib installed on my system that was always used, instead of the one I specified using the LLVM_DIR option.

The second commit adds support for strings in expressions. Now expressions can correctly return strings (through the evalStr method), user functions can also return strings, and user variables can be strings. I didn't test with evalMultiple or VarBlocks though ...

Anyway, let me know what you think of this, I can update this pull request as much as needed :)

[davvid]

Thanks @dcourtois this looks great. Thanks for the cmake fixups too, they should work just fine in our setup too.

It shouldn't hold up our merge of this PR, but it might be helpful to add a simple unit test cpp that defines an expression function (that returns and takes strings) and then run some assertions using some example expressions on the code to verify that the new feature is working as expected.

Thanks again!

[ix-dcourtois]

I'll add some unite tests and update the PR when done, thanks for the feedabck !

[ix-dcourtois]

Hi again ! I've been trying to add unit tests without much success.

First I had a few fix in the tests/CMakeLists.txt file to find libPng: the script looks for libraries in ${PNG_DIR}/${CMAKE_INSTALL_LIBDIR} which feels a bit odd ... I replaced it by ${PNG_DIR}/lib and it works fine, but again, I don't know how it would behave in your setup.

The next problem I had is that I don't know which version of gtest you're using. I tried the github hosted one, but I have a bunch of compilation errors.

So I will need a little bit of information to be able to add the unit tests for string expressions :) Thanks in advance and have a good week-end ! (it's already the end of the day here :p)

[davvid]

For gtest, target the latest version hosted on github. We enable their shared libraries (-DBUILD_SHARED_LIBS=ON) and link our tests against the shared gtest.

For libpng, if CMAKE_INSTALL_LIBDIR is a relative path then that should work, but that's assuming that libpng was built with the same basename for its lib directory. It's probably pretty easy to break that assumption.

A way to make that more configurable would be to maybe default libpng's library dir to that expression, and store it in a variable that can be overridden when invoking cmake.

The libdir stuff is kinda annoying in that we're following current RHEL 6/7/.. model where libraries go in $prefix/lib64, so we accommodate for that. I use debian/testing at home so I try to at least get it so that it has sensible behavior on those two platforms w.r.t how the default paths are configured. On debian and ubuntu it's $prefix/lib.

If CMAKE_INSTALL_LIBDIR is configured to an absolute path then I can see how that would not work, as its default is just the basename (e.g. lib). Is another possibility that the expression needs to take the basename of the libdir path before joining them?

[ix-dcourtois]

New week started, new test added :) I ran testmain2, and I just had one fail on paint3d_test_examples.dither_with_gamma, but it also happens in the master branch, so I guess it's not related to my modifications.

And I have a semi-related question : in the case of the LLVM evaluator, wouldn't it be simpler to use _parseTree->type() instead of _desiredReturnType ? That would simplify the use of Expression since the type would be correctly inferred from the expression itself, and the user wouldn't have to bother setting it.

[ix-dcourtois]

It makes perfect sense, thanks !

[ix-dcourtois]

Small update: there was a crash in the Interpreter when evaluating string type ExprVarRef, the last commit fixes it.

[davvid]

@jberlin I merged this in. We can work on integrating and testing it further next week. Thanks again @ix-dcourtois !