Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

call function, first argument is a matrix #23

Open
thorade opened this issue Oct 21, 2015 · 13 comments
Open

call function, first argument is a matrix #23

thorade opened this issue Oct 21, 2015 · 13 comments

Comments

@thorade
Copy link
Collaborator

thorade commented Oct 21, 2015

Don't know how to describe this. The highlighting is somehow broken here:
http://bit.ly/1ZXaFdK

This is how it looks with the v0.2.1 grammar:
http://bit.ly/1ZXaLlu
Also not sure if that is the intended behavior.

within ;
model SurfaceToAir "cool model"
  Modelica.Electrical.Analog.Basic.Resistor inRes[3,3];
  Modelica.Electrical.Analog.Basic.Resistor outRes[3,3];

equation 
  for j in 1:3 loop
    for k in 1:3 loop
      if inRes[j,k].R > 0 then
        outRes[j,k].R = Modelica.Math.cos(inRes[j,k].R - inRes[k,j].R);
      elseif inRes[j, k].R > 2 then
        outRes[j,k].R = Modelica.Math.cos( inRes[j,k].R - inRes[k,j].R);
      elseif inRes[j, k].R > 3 then
        outRes[j,k].R = Modelica.Math.sin(inRes[j, k].R - inRes[k,j].R);
      else
        outRes[j,k].R = 0;
      end if;
    end for;
  end for;
  annotation (uses(Modelica(version="3.2.1")));
end SurfaceToAir;
@bilderbuchi
Copy link

If you change the line to outRes[j,k].R = Modelica.Math.cos(inRes[j, k].R - inRes[k,j].R); (space in the first square bracket), it starts to work, so I guess the regex responsible for the square brackets needs fixing (maybe has +instead of *).
Also, it works if there's a space after the parens, i.e. cos( inRes[j,k].R, and no space in the square brackets, so that's probably the interaction that goes wrong.

@thorade
Copy link
Collaborator Author

thorade commented Oct 21, 2015

PS: Lightshow is very nice for testing grammars, you can even copy and paste the full grammar file content and/or the full file content. That way one can edit and test grammars quite fast.
If you want to work on it, I will happily merge a pull request or we can ask the repo administrator to add you as a collaborator.

@bilderbuchi
Copy link

my suspicion is that the keyword regex is the culprit. for example, the double ++ near the end looks strange to me (but could be a cson detail?) - If I remove that, and test if the keyword gets recognized, it nearly shows the behaviour in the lightshow (but not exactly). I think it just doesn't account for parentheses, and this would have to be added (but I never edited atom grammas before)!

p.s. I'd be happy to PR, but I have to go now. btw, automated tests for these grammars/highlighting would be nice, so that the test cases don'T get lost in issue comments, and we can ensure we don't create bugs elsewhere.

p.p.s: didn't realize that you could paste in lightshow, too - that's probably the best way to iterate this! thanks!

@thorade
Copy link
Collaborator Author

thorade commented Oct 21, 2015

@bilderbuchi
Copy link

So, it turns out this is called "possessive repeat": "++ Matches the previous atom one or more times, while giving nothing back." (i.e. without backtracking)

@bilderbuchi
Copy link

yeah, sorry, I tinkered a bit, no idea how to correct this. I think the key is that the keyword regex falsely matches the string Modelica.Math.cos(inRes[j,k].R, when it should not match it at all. A thing to think about is if strings like Modelica.Electrical.Analog.Basic.Resistor should also be highlighted red (like it is now), or if this is also an error. If the latter, this keyword regex gives better results (make the repetition of the first groups also possessive with ?+): "\\b\\s*([A-Z])(?:([^ ;$]+)(;)?+)([.]([A-Z])(?:([^ ;$]+)(;)?)?)++\\b (but it's still necessary to check if this breaks other source examples!).

@raphaelchenouard
Copy link
Collaborator

I'm not sure to understand what you want to highlight:
(1) only: Modelica.Math.cos
(2) Modelica.Math.cos and inRes.R (without [j,k])
(3) None

Here is a regex for (1): \\b\\s*(([a-zA-Z])([a-zA-Z0-9])*)(\.(([a-zA-Z])([a-zA-Z0-9])*))+\\b

@bilderbuchi
Copy link

I think the highlighting of the first occurrence should match the other two occurrences of Modelica.Math.[cos|sin] here (i.e. only the built-in functions should be highlighted, in blue).
Btw, what is \\b\\s*supposed to be doing? How/when can a space follow a word boundary?

@raphaelchenouard
Copy link
Collaborator

I think \\b is enough.

@thorade
Copy link
Collaborator Author

thorade commented Oct 26, 2015

@bilderbuchi
Could you send a Pull Request with the regex you proposed above?
Do you have ideas how to automize regression testing?

@bilderbuchi
Copy link

I have this on my todo list, but I'm pretty busy currently.

@bilderbuchi
Copy link

no ideas about automated testing, though. people seem to use "specs" to do testing of grammars, at least that's what it looks like to me (I have no idea how to do this).

@bilderbuchi
Copy link

no ideas about automated testing, though. people seem to use "specs" to do testing of grammars, at least that's what it looks like to me (I have no idea how to do this).

hm, it seems @tshort knows a thing or two about specs, maybe he can help with setting up automated testing for this?

@thorade thorade changed the title call function, opening parantheses call function, first argument is a matrix Oct 29, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants