-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
grc: refactor c++ generation and fix zero-port busses #7072
Conversation
Seems like too much to put into 3.10.9.2. Opinions? |
Ha! I might be biased, and would opine otherwise 🗡️ But let's look at that in detail: Commit grc: fix C++ gen with busports fixes the C++ generation issue and completes #7067 Commit grc: minor refactoring of cpp_top_block is but minor mechanical refactorings, which don't need to be backported, but would make Commit grc: fix busport creation with 0 ports is a bit "raw" in that it combines a bugfix (which should be backported, but isn't very high prio) with refactoring. That's mostly due to me having to do the refactoring to be able to fix the bug. I can look into extracting the quality-of-life improvements out of that commit (that is, mainly, consistent string quoting/f-strings), but it'd be sad to also remove the added logging, which was helpful knowing where things went wrong. The actual refactoring was rather benign, in essence: extract a function and convert def handle_ports(self, ports):
for thing in {"foo", "bar"}:
if thing == "foo":
baz = self.a[thing]
elif thing == "bar":
baz = self.b[thing]
a_lot_of_code_here_using_ports_and_baz to def do_thing(self, baz):
a_lot_of_code_here_using_ports_and_baz
def handle_ports(self, ports):
if not ports:
return_early
self.do_thing(ports, self.a["foo"])
self.do_thing(ports, self.b["bar"]) |
I'll give it a closer look. I was hoping we could keep the 3.10.9.2 changes down to really small things that are easily checked in a diff. |
Amended last commit with python formatting, cleaned up commit message. |
@@ -13,7 +10,7 @@ | |||
|
|||
DATA_DIR = os.path.dirname(__file__) | |||
|
|||
PYTHON_TEMPLATE = os.path.join(DATA_DIR, 'flow_graph.py.mako') |
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.
Why change all '
to "
?
Can you point out the bugfix for 0-length bus ports? The ' to " conversion makes this diff hard to read. |
@@ -492,10 +506,10 @@ def by_domain_and_blocks(c): | |||
for port_num in porta.bus_structure: | |||
hidden_porta = porta.parent.sources[port_num] | |||
hidden_portb = portb.parent.sinks[port_num] | |||
connection = fg.parent_platform.Connection( |
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.
This is the line equivalent to the Python fix.
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.
Yeah, I found that -- and it's also nicely put into its own commit. It seemed like there's another bugfix here regarding 0-length bus ports...? What's even the bug?
if not ports: | ||
self.current_bus_structure[direc] = None | ||
return |
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.
This is the core fix, @mbr0wn
sure, @mbr0wn , #7072 (comment) ; but find that in the "from" code… |
- clean up includes - active block filtering refactor - variable type mapper refactor - param type conv refactor Signed-off-by: Marcus Müller <mmueller@gnuradio.org>
needed refactoring: short circuit zero port behavior, use f-strings, actually handle Exceptions, log things, consistent quotation marks, sorted stdlib includes… Signed-off-by: Marcus Müller <mmueller@gnuradio.org>
Split simple fix commit out to another PR for quick merge. We need to back out the unneeded |
Do portions of this still apply after the related PR that was merged? |
@willcode good question, don't have resources right now to look at that in detail
You could right-click on a block with no output ports, and select "create bus source port", and it would suddenly get a (very confusing, and dysfunctional) white bus output port, combining all 0 ports into 1 bus port. (same for sink) |
Description
Busports flowgraphs couldn't be generated. Martin fixed the Python case, this fixes the C++ case.
As a drive-by, this untangles a bit of the block.py code, which was necessary for me to actually understand it.
Related Issue
Fixes #7069
Which blocks/areas does this affect?
GRC
Testing Done
made https://gist.github.com/marcusmueller/5cd33ac2f6cf752049ddf7a50b7bc974 compile
Checklist