modelica-tools / atom-language-modelica

Modelica language support for the atom editor
https://atom.io/packages/language-modelica
Other
8 stars 4 forks source link

call function, first argument is a matrix #23

Open thorade opened 8 years ago

thorade commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

The issue is also present in the Sublime Text grammar: https://github.com/BorisChumichev/modelicaSublimeTextPackage/issues/10 The ++ is not a cson detail I think. https://github.com/BorisChumichev/modelicaSublimeTextPackage/blob/master/Modelica.tmLanguage#L53 https://github.com/BorisChumichev/modelicaSublimeTextPackage/blob/master/Modelica.YAML-tmLanguage#L30

bilderbuchi commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

I think \\b is enough.

thorade commented 8 years ago

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

bilderbuchi commented 8 years ago

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

bilderbuchi commented 8 years ago

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 commented 8 years ago

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?