PSyclone icon indicating copy to clipboard operation
PSyclone copied to clipboard

only_exact_name parameter for find_or_create_tag on SymbolTable

Open LonelyCat124 opened this issue 1 year ago • 3 comments

When creating variables which need a tag (to find later) and we need to guarantee that the created symbol will have the correct name (e.g. if they are an import from a module) an only_exact_name parameter would be helpful.

If this cannot be met I think we just throw an exception and fail for now.

LonelyCat124 avatar Apr 05 '24 13:04 LonelyCat124

Can't you rename the imported symbol?

hiker avatar Apr 05 '24 13:04 hiker

I'm not sure if you're allowed to rename things like c_bool from iso_c_bindings - probably we can but I would prefer to force it not to and to fail instead.

LonelyCat124 avatar Apr 05 '24 14:04 LonelyCat124

Both ifort and gfortran are fine with that:

  use iso_c_binding, only: my_c_bool => c_bool
...
  logical(kind=my_c_bool) :: cb

But fwiw, here what I have used elsewhere (signature[0]is a string of the symbol name) to handle the renaming:

            if signature[0] in symbol_table:
                interface = ImportInterface(container, orig_name=signature[0])
            else:
                interface = ImportInterface(container)

            symbol_table.find_or_create_tag(
                tag=f"{signature[0]}@{module_name}", root_name=signature[0],
                symbol_type=DataSymbol, interface=interface,
                datatype=container_symbol.datatype)

hiker avatar Apr 05 '24 14:04 hiker

Both solutions are fine and it's up to the caller to decide what it wants: renaming-import or exact name only.

The only thing that is not good is that currently the find_or_create_tag (the new_symbol method really) renames imported symbols (without using the renaming-import). So, I will ask to add the following check in @LonelyCat124 PR

if 'interface' in new_symbol_args:
    if isinstance(new_symbol_args['interface'], ImportInterface)
        if not new_symbol_args['interface'].orig_name:
            if root_name in self.get_symbols().keys():
                raise error

Instead of raise error it could be new_symbol_args['interface'].orig_name = root_name and then it will do your if/else logic inside the method. Do you think this could be useful @hiker or you want it outside the find_or_create_tag call?

Also, we need to check that is not UnsupportedFortranType as these also include the name inside them and can't be renamed.

sergisiso avatar Apr 08 '24 13:04 sergisiso

[...]

            if root_name in self.get_symbols().keys():

I don't think you need keys() here.

Instead of raise error it could be new_symbol_args['interface'].orig_name = root_name and then it will do your if/else logic inside the method. Do you think this could be useful @hiker or you want it outside the find_or_create_tag call?

Since the renaming on import is the exception, I would actually suggest to just leave it to the caller to handle this (i.e. the if-test is visible and can easily be commented). Implicit renaming when exact_name is requested feels very confusing. If we want to have this all done automatically, then we should at least have an explicit argument (e.g. exact_name="enforce" or "rename" or so??)

hiker avatar Apr 10 '24 14:04 hiker

The implementation I have done is:

  1. There is an allow_renaming flag that defaults to True. If its False or the datatype is an UnsupportedFortranType then we throw an error if available_name != root_name.
  2. If the new symbol has an ImportInterface, and available_name != root_name, then we add the orig_name argument to the ImportInterface (or well, we create a new ImportInterface but its essentially the same). This means we automatically handle renaming imports if needed. I think this is a sensible thing for the symbol_table to be able to do given its not difficult to handle.

LonelyCat124 avatar Apr 10 '24 14:04 LonelyCat124