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

fix VhdlExtractor.is_array + use setup.cfg + clean #10

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

kammoh
Copy link

@kammoh kammoh commented Mar 9, 2022

  • start using setup.cfg
  • fix VhdlExtractor.is_array (used in symbulator) which was completely broken
  • avoid adding log handler (unless no handlers already exist), to avoid messing up caller's logger.
  • verilog_parser: more rebust endmodule identification
  • fix identification of file extension (e.g. my.module.v)
  • consider '.sv' file extension as (System)Verilog

umarcor
umarcor previously approved these changes Mar 10, 2022
Copy link
Member

@umarcor umarcor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@umarcor umarcor requested a review from michael-etzkorn March 10, 2022 09:07
Copy link
Collaborator

@michael-etzkorn michael-etzkorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to work on getting #2 merged so we can skip the minor edits on conf.py. @umarcor what do you think? I'm noticing the conf.py changes would cause unnecessary merge conflicts. The setup changes run okay on my end. With umarcor/doc-btd, I imagine we're changing the url and repo-name to point to our fork.

I mentioned this in a comment: Adding parsing of .sv file extensions is a bit disingenuous because we don't even parse logic. I've hacked around this when using the parser by adding logic to the module and module_port regexes, but it really should be some API flag with extract_objects* or with VerilogExtractor's constructor.

I'll look over the vhdl_parser functional changes this afternoon.

@@ -226,7 +221,7 @@ def is_verilog(fname):
Returns:
True when file has a Verilog extension.
"""
return os.path.splitext(fname)[1].lower() in ('.vlog', '.v')
return os.path.splitext(fname)[-1].lower() in ('.vlog', '.v', '.sv')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The verilog parser currently offers next to no SystemVerilog extension support, so I'm not sure we should add .sv here unless we intend to start adding minimal support. The problem is even if all we did was parse logic, I'm pretty sure something like wire [7:0] logic is valid verilog... We'd ideally want to make it an optional flag.

I am in favor of basic support as I mention here:
#4 (comment)

but we can maybe start a different branch / PR for that?

project = u'Hdlparse'
copyright = u'2017, Kevin Thibedeau'
author = u'Kevin Thibedeau'
project = 'Hdlparse'
Copy link
Collaborator

@michael-etzkorn michael-etzkorn Mar 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The small edits here will conflict with umarcor/doc-btd, but I'm not sure the status of that PR. I'd ideally merge that before doing clean-up here. It seems our intention is to change the name to pyHDLParser since I don't think we can get the PyPI namespace unless we hear back from Kevin.

Copy link
Collaborator

@michael-etzkorn michael-etzkorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't used the vhdl parser much outside of toy tests since I don't touch much vhdl in my day-to-day. From Kevin's documentation, I ran the following is_array test:

import hdlparse.vhdl_parser as vhdl

vhdl_ex = vhdl.VhdlExtractor()

code = '''
package foobar is
  type custom_array is array(integer range <>) of boolean;
  subtype custom_subtype is custom_array(1 to 10);
end package;
'''
vhdl_comps = vhdl_ex.extract_objects_from_source(code) # TODO: is extract_objects in doc 

# These all return true:
print(vhdl_ex.is_array('unsigned'))
print(vhdl_ex.is_array('custom_array'))
print(vhdl_ex.is_array('custom_subtype'))

I haven't looked at Symbolator yet. This could be something on that code's end as far as how it is using the API (passing in some class with attribute name?), but this conflicts with the documentation's current API. Personally, I think I prefer passing in strings to is_array. If we were to change the API, we have to update that part of the docs as well.

if s:
n = 1
while n:
s, n = re.subn(r'\([^()]*\)', '', s.strip()) # remove non-nested/flat balanced parts
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of things here:

  • This regex seems to remove anything nested in parentheses which isn't what its name suggests.
  • Parenthesis should be plural parentheses
  • This function isn't called anywhere and isn't in a class?

data_type = data_type.split('[')[0].strip()

return data_type.lower() in self.array_types
return data_type.name.lower() in self.array_types
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing in vhdl_ex.is_array("unsigned") now fails because string does not have an attribute name.

iDoka added a commit to iDoka/hdlparse that referenced this pull request Feb 20, 2024
iDoka added a commit to iDoka/hdlparse that referenced this pull request Feb 20, 2024
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

Successfully merging this pull request may close these issues.

3 participants