Codecov Report

Merging #293 (4d8228a) into master (6558244) will increase coverage by 0.13%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #293      +/-   ##
==========================================
+ Coverage   90.95%   91.09%   +0.13%     
==========================================
  Files          35       36       +1     
  Lines       12917    13107     +190     
==========================================
+ Hits        11749    11940     +191     
+ Misses       1168     1167       -1     

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6558244...4d8228a. Read the comment docs.

Hi @rupertford, this isn't finished yet but would you mind having a glance over it just to confirm you're happy with the direction?

Hi @rupertford, this isn't finished yet but would you mind having a glance over it just to confirm you're happy with the direction?

Hi @arporter. It looks great. I really like the scoping, symbol tables singleton and symbol table hierarchy. My only comments/questions are

1. I presume we will have to support different types of symbols. I see that you capture module use separately, which is nice and simple but shouldn't the module name be added as a symbol (in a future PR)?
2. is capture_matched_symbol the best way to go? It is very nice but its interface is generic so it means that the context needs to be determined within the function and given that the context will be known when matching (as the classes are specific) it seems like it would be easier to have the logic associated with the class itself. I think this will become more true as we support more symbol types (assuming that is what we do).
3. Would it be useful to have a str for SymbolTables as well as SymbolTable?
4. Do we need a singleton for SymbolTables, i.e can we always create one instance at the "start" and just use it? We would then not need to do .get() all the time.
5. It seems a shame to have to add so many symbol table decorators around tests, but I guess that is just the way it is as there is no scoping information.
6. Do we need a "full name" for a symbol table given that they are in a hierarchy?

re. your question 6., not unless we want only the top-level symbol tables to by stored in SymbolTables. Otherwise we would get a clash, e.g. if two different modules contained a subroutine with the same name. I'm not sure what the best approach is although I'm tempted to say that perhaps we should only store the top-level tables. Should we ever need to look at all symbols (and I can't think why we would), we could go through each of these and then down the hierarchy that they contain.

re. your question 6., not unless we want only the top-level symbol tables to by stored in SymbolTables. Otherwise we would get a clash, e.g. if two different modules contained a subroutine with the same name. I'm not sure what the best approach is although I'm tempted to say that perhaps we should only store the top-level tables. Should we ever need to look at all symbols (and I can't think why we would), we could go through each of these and then down the hierarchy that they contain.

Yes, that is what I was wondering. If we store the tables in a hierarchy and don't have a dictionary lookup then we would not need a "full name".

re. your question/comment 5. I think it would be possible to silently ignore the symbol table functionality if ever current_scope came back as None. However, that's letting the implementation be twisted so as to fix a testing issue and that's often not a good thing.

This is now ready for a proper review. Note that although use statements are captured, any symbols named in 'only' clauses are not currently captured as symbols. Probably we will need a class to represent Symbols and this could have a module property if the symbol is imported from a module. That would then be the key for the symbol table containing the symbol definition. Anyway, that's something for the next PR.

This is ready for another look now @rupertford.

I'm not sure what to do about one of the diff coverage problems (L595-596 in utils.py) - it is code that I've touched in that I've now surrounded that block of code with a try, although I've not altered the code itself (apart from indentation, obviously). I've run the parser over the whole of NEMO and the un-covered condition is never triggered, I think because Label_Do_Stmt never raises a NoMatchError - it just returns None if there's no match. Therefore, I could remove the try...except around the match but I feel a bit uneasy about that. What do you think?

In running this branch on the NEMOVAR code base I discovered an error that occurs when trying to remove a symbol table after a syntax error has occurred - I was only looking at the list of top-level tables and not looking within the current scope. I've updated the code to do this and now I think all syntax errors are related to overridden intrinsics.

OK, ready for another round. I've decided I don't understand the BlockBase.match() method but have succeeded in removing a couple of flags that don't appear to do anything useful. I've tested with NEMO and NEMOVAR and it all seems fine. That said, I'll try using PSyclone with this version of fparser and see what happens.

I can confirm that I can process and then compile NEMO using this version of fparser and master of PSyclone.