Closed EverythingElseWasAlreadyTaken closed 4 months ago
I have done something similar recently. You can check out here. If this is helpful for you I will make a pull request publish that out.
I have done something similar recently. You can check out here. If this is helpful for you I will make a pull request publish that out.
Thanks for the hint, but there might be some minor issues with your changes in the parseFileVerilog/VHDL. For the parseFIleVHDL, you still have this big chunk of if-else statements, which is already fixed in the parseFileVerilog. Also in the parseFileVerilog, you are treating INOUT ports always as OUTPUT ports, since you only check for INPUT ports.
The other changes are quite nice, we should somehow combine them. Or you could maybe merge this PR and rebase your commit to this.
I've also though about combining the parseFileVHDL and parseFileVerilog in one function, since the code is like 80% same. This would improve the maintainability and reduce the likelyhood for errors.
The main problem with combining is the subtle difference in parsing. You will have one blob for Verilog and another for VHDL, which might make the code look bulky again. But this should indeed reduce the amount of things that need maintenance.
I will rebase mine on yours later. But I have other things to do first, so that won't happen soon.
The ideal way would be to do it in a parser way, which reads in both Verilog and VHDL, and then we extract data under a common AST... But this in itself will be a project.
The main problem with combining is the subtle difference in parsing. You will have one blob for Verilog and another for VHDL, which might make the code look bulky again. But this should indeed reduce the amount of things that need maintenance.
Yes, but I think this will mostly concern the regex lines. Should I create an Issue for this, so this doesn't get lost?
I will rebase mine on yours later. But I have other things to do first, so that won't happen soon.
Okay. Sure, there is no rush.
The ideal way would be to do it in a parser way, which reads in both Verilog and VHDL, and then we extract data under a common AST... But this in itself will be a project.
Yes, this would be really nice, but this will be indeed a big project.
Since you are refactoring, you can include it here as well and rename the PR to mention refactoring both.
Since you are refactoring, you can include it here as well and rename the PR to mention refactoring both.
Okay, I can do this. :+1:
Since you are refactoring, you can include it here as well and rename the PR to mention refactoring both.
I've merged them into one parseFile function now. Would be nice if you could review my changes. :)
@KelvinChung2000 have you already had some time to look at it again?
I have some other works that are partly based on this.
I have put some comments on it. I am not sure why when I made an review it did not notify you
Does it work now?
Does it work now?
Yes, saw the Review now, but I didn't get any notifications before this.
Thanks, I'll look into this :)
Refactor parseFileVHDL to make it more readable. This fixes also the bug, that shared INPUT ports in VHDL files, were added as INOUT ports.
Add port direction variable in parseFileVerilog function to make it more readable.
Some cosmetic changes.
Move commit 8cb8aab (Make expandListPorts public) from #176 to this PR.