Update ExprTk

[ArashPartow]

1. Update ExprTk to 0.0.3 Release notes: https://www.partow.net/programming/exprtk/exprtk_release_notes_v0.0.3.txt
2. Adaptor update
3. Minor fixes and cleanups in exprtk, scalar, computed functions

[ArashPartow]

@texodus Looks like a couple of adaptor methods and free functions need to be added to perspective::t_tscalar:

1. to_uint64
2. cast operator to uint64 from type

I'm ok with adding them, would you want it all in the same PR or two PRs, initially the additional adaptor methods and then this PR?

[texodus]

One PR would be ideal.

[texodus]

I'm not sure if you can see our CI logs externally, but this PR does not compile for different reasons on every architecture we support.

I have not had time to test this on anything but my development machine yet, but I've rebased your PR and fixed the local compilation errors here, which you can cherry pick.

[ArashPartow]

Finally got all the tests passing. Minor issues with some of the test results revealed a parsing issue in ExprTk.

That being said there is a change I couldn't resolve:

-    struct random : public exprtk::igeneric_function<t_tscalar> {
+    struct random final : public exprtk::igeneric_function<t_tscalar> {
         random();
-        ~random();
+       ~random();

-        t_tscalar operator()(t_parameter_list parameters);
+        t_tscalar operator()(t_parameter_list parameters) override;

         // faster unit lookups, since we are calling this lookup in a tight
         // loop.
-        static std::default_random_engine RANDOM_ENGINE;
-        static std::uniform_real_distribution<double> DISTRIBUTION;
+        std::mt19937 generator;
+        std::uniform_real_distribution<double> distribution{ 0.0, 1.0 };
     };
 -// Set up random number generator
-std::default_random_engine random::RANDOM_ENGINE = std::default_random_engine();
-std::uniform_real_distribution<double> random::DISTRIBUTION =
-    std::uniform_real_distribution<double>(0, 1);
-
-random::random() : exprtk::igeneric_function<t_tscalar>("Z") {}
+random::random()
+: exprtk::igeneric_function<t_tscalar>("Z")
+{
+    std::random_device device;
+    std::array<unsigned int,std::mt19937::state_size> seed;
+    std::generate_n(seed.data(), seed.size(), std::ref(device));
+    std::seed_seq seq(std::begin(seed), std::end(seed));
+    generator.seed(seq);
+}

It seems the use of std::random_device in a WASM/NODERAWFS context is the issue. The implementation of std::random_device is typically done by using /dev/random or /dev/urandom (at least in a posix context). However if those devices are not available, this leads to UB which resulted in spurious crashes during the js tests. I tried several approaches to getting it to work, however was not successful.

The main reason for this particular change was to properly seed the engine, as the way it is currently being setup is not correct. It will use an internal state based on whatever is in the memory contents of the allocation. Which could be random or all zeros etc.

[texodus]

Thanks for the PR! Looks good!

[texodus]

👀

[ArashPartow]

Yeah the ratio for converting between degrees to grad is 1.1111...

[texodus]

I checked the Perspective raycasting example, which does not compile with a syntax error on this branch. It can be fixed by inserting a semicolon on line 58. I presume this was a parser error that was incorrectly accepted in 0.0.2, as we are not enabling settings_t::e_commutative_check.

[ArashPartow]

I presume this was a parser error that was incorrectly accepted in 0.0.2, as we are not enabling settings_t::e_commutative_check.

Yeah that's correct, the fixes in 0.0.3 tighten up a lot of the checks - including when e_commutative_check has been disabled.

The way the parser is being configured (without e_commutative_check) currently in perspective is the best option, as the implied multiplication functionality can be surprising for people that don't come from a maple/mathematica et al. background.

[texodus]

I am having some trouble merging this PR. First off, GitHub's merge UI seems to timeout trying to validate it, which I've never seen before - all acceptance checks are green, but it is stuck "Checking for ability to merge automatically…".

So I tried to merge this locally and noticed my local Git client behaving strangely, e.g., there are only ~10 contemporary PRs in flight right now on the repo, but when adding yours as a remote git log suddenly decides 60+ old branches are now newer than yours, causing lineage views like this one to become unreadable. Snippet showing the git log --graph depth:

| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | * 05e887855 (@ArashPatow/arashpartow/update_exprtk_0.0.3) Update ExprTk
| |_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|/
|/| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | |
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | * a5c157ebf (@ArashPatow/arashpartow/update_exprtk_0.0.3_trials) Update ExprTk - Investigation
| |_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|/
|/| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | |

Merging this locally (and even removing your remote) retains the date-ordering bug. Rebasing your PR on master locally and merging does not, however.

Your PR only diverges from master by 3 commits, so I'm not sure what's going on here. The date is suspicious however - Dec 31st 12:00 UTC 2023 - makes me think some metadata in the commit is corrupt or out-of order. I am not a GIT expert (clearly).

Can you please try rebasing/cherry-picking or otherwise recreating this commit on the latest master? Alternatively, I can rebase this PR locally and merge it, the commit will still carry your attribution but the GitHub PR may say "closed".

[ArashPartow]

@texodus I've rebased my PR to: 452bfb0eb (master) Merge pull request #2842 from finos/fix-integer-cast-function

If it still doesn't merge cleanly and without errors, I'm happy for you to cherry-pick and PR it locally and merge 👍

patch url: https://github.com/finos/perspective/commit/2340bdb85101415d4e41648211189841c727e6f0.patch

[texodus]

That worked, thanks!