possibble elegant solution to bug 1165, just need valueId to take on …#165
Conversation
…replacement characters in new proc subSpecChars
|
I don't see why we'd need to do this at all. Setting a variable's name to include things like \ and { has never been a problem. The problem has been with valueIds, which are used as the key of a map. The function that turns a name into a valueid is stripActive(), which can be found in the file str.h. It is, of course, not 1:1, and this is the root cause of the problem. An alternative that might be worth trying is to use latexToPango(). This is not 1:1 either, but it will be so in the right way. If a valueid is the same after latexToPango, then it will have the same appearance, ie x_y and x_{y} will have the same valueId, but x_yz and x_{yz} will not. |
|
Another thing you could try is simply to use the variable's name as valueid. It may well be that in the Minsky 2.0 refactor, there is no longer a problem in doing this, like there was earlier. Unfortunately, I can't remember exactly what the problem was. |
|
Whoops. I thought the remedy to the problem was in TCL. I was hoping the valueIds would also contain the substituted characters, thus making the 1:1 correspondence complete. I'm trying the third solution you suggested now, thanks:) |
… still fails though, but don't want to do a volkswagen and MAKE the test pass
str.h
Outdated
| switch (s[i]) | ||
| { | ||
| case '\\': case '{': case '}': | ||
| case '\\': //case '{': case '}': |
There was a problem hiding this comment.
So braces are now included in valueIds, but not backslashes. Did you identify there were problems with backslashes?
Also spaces are still confused with underscores. So "a_b" and "a b" map to the same variable.
What did you think about using latexToPango as the conversion string? The only time a backslash would appear in the output is if the user types "\backslash". And there is probably a unicode replacement we could use.
There was a problem hiding this comment.
So braces are now included in valueIds, but not backslashes. Did you identify there were problems with backslashes?
Yes, the test compareSimulationWithLogged.sh fails because of missing backslashes.
What did you think about using latexToPango as the conversion string? The only time a backslash would appear in the output is if the user types "\backslash". And there is probably a unicode replacement we could use.
Yes, I replaced stripActive in VariableValue.h, VariableValue.cc and equations.cc by latexToPango, but the assert on line 625 of equations.cc: assert(minsky.variableValues.count(valueId)); fails. I commented it out to see what would happen, but Minsky crashes. I will debug it today.
When I couldn't make progress on adding the Curl Requests C++ wrapper as a submodule yesterday (damn cmake!), I switched back to this bug, but then after I got stuck, I switched to ticket 1163 (and 1166 which I think is related to it), but I still haven't identified the cause. I will GDB both today.
… origin, also valueId related, led me here
model/godleyIcon.cc
Outdated
| { | ||
| bool operator()(const VariablePtr& x, const VariablePtr& y) const | ||
| {assert(x&&y); return x->valueId() < y->valueId();} | ||
| {assert(x&&y); return x->name() < y->name();} // Characeters stripped from valueIds by stripActive() seem to cause this to not work properly. For ticket 1165. |
There was a problem hiding this comment.
This will fix the bug you're targetting, but will create another one. In a global godley table, variables "foo" and ":foo" are in fact the same variable. With this change, they will be treated as different variables. In godley tables stashed in a group, "foo" and ":foo" will be different variables.
The problem is clearly due to distinctly different variables having the same valueId, which is wjat ticket #729 is about.
There was a problem hiding this comment.
Oy vey, that's true. OK, putting a latexToPango around the valueId seems to work too. So, I've done that. Just running "make sure" on my machine now to check that all tests pass and will commit the code soon after. I will also try the approach of putting latexToPango around valueIds when they are created and hopefully this will get around the failing tests.
There was a problem hiding this comment.
Errm, I was wrong... I made the changes but failed to compile before running Minsky to test it. After compiling via make sure I decided to double check and the bug was back... I really wanted to kill this one today, but I'm out of steam after battling with the installation of curl too. (I've started working on a TCL implementation that will import CSVs from URLs, because in the best case scenario, users will have to install curl's libraries on their system if we wanted to go that way. I could not get it to work as a submodule and had to install the dev package separately to get access to the curl.h header).
There was a problem hiding this comment.
Readjusting the saved logs to account for changes to the value id is not "Volkswagening". That should be the only impact on the tests IIUC.
There was a problem hiding this comment.
Done. The tests all pass now and hopefully they will on Travis too.
|
OK lets suck it and see. |
Also added publication tab C++ code for first time.
2 participants
…replacement characters in new proc subSpecChars. I tested the implementation at https://rextester.com/MRYAO95610 and used code I found at https://www.tcl.tk/man/tcl8.4/TclCmd/regsub.htm. What I don't understand is why valueId's don't use the replacement character ¿ I chose (and think no user would ever use)?
This change is