-
Notifications
You must be signed in to change notification settings - Fork 10
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
base: main
Are you sure you want to change the base?
Conversation
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this 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') |
There was a problem hiding this comment.
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' |
There was a problem hiding this comment.
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.
There was a problem hiding this 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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.