stfc / PSyclone

Domain-specific compiler and code transformation system for Finite Difference/Volume/Element Earth-system models in Fortran
BSD 3-Clause "New" or "Revised" License
102 stars 27 forks source link

Unnecessary walk_ast calls in nemo api? #397

Closed hiker closed 5 years ago

hiker commented 5 years ago

To find the name of the subroutine, the following code is used:

          # Get the name of this (sub)routine
            substmt = walk_ast(subroutine.content,
                               [Subroutine_Stmt, Function_Stmt, Program_Stmt])

As far as I can see the walk_ast is not necessary, the substmt should always be

substmt=[subroutine.content[0]]

The unit tests seems to work fine with this change (though afaik they do not test Function_Stmt). Is there a reason for the walk_ast? If not, I am happy to adjust this (and add a test for function_stmt).

There is also at least one location where a walk_ast is done, but only the first element is ever needed, e.g.:

  names = walk_ast(ast.content, [Fortran2003.Name])
  # The name of the program unit will be the first in the list

In my example code the list of names has over 3000 elements. Would it be worth to add a 'max_number_of_elements' parameters to walk_ast? I.e. we could then specify to return only 1 element in those cases, significantly reducing the amount of work required in nemo. This would then also be useful for the case at the top (if we can't replace the walk_ast to get substmt) ... which is why I added both suggestions here in one ticket.

arporter commented 5 years ago

TBH, I can't remember why I did a walk_ast() - probably because I wasn't confident that the first child was the one that always contained the name. However, if you are then we can do away with the walk.

For the second part of your question (and again, this probably influences the first part) I think we need to alter walk_ast() to use yield rather than build up a static list of matches before we return. However, I've never used yield so I'm into unknown Python territory here...

hiker commented 5 years ago

I am not going to touch walk_ast itself (since this is in fparser), but am going to replace the one unnecessary walk in nemo.