Fix anno in backend #428
Fix anno in backend #428
Conversation
Added a few more tests
Only create .f file if some files have been moved Some small style fixes in Driver Restored lost functionality to add -f argument in verilogToCpp Fixed loadAnnotations to add targetDir regardless of annotations arriving from file or through options
| file.getAbsolutePath == "/" || | ||
| file.getPath.startsWith("/")) { | ||
| Driver.dramaticError(s"delete directory ${file.getPath} will not delete absolute paths") | ||
| false | ||
| } |
There was a problem hiding this comment.
Could we not use deleteOnExit() instead? https://docs.oracle.com/javase/7/docs/api/java/io/File.html#deleteOnExit()
There was a problem hiding this comment.
deleteOnExit only deletes a directory if it is empty. I don't see any particular value to using it over delete.
There was a problem hiding this comment.
I guess what I mean is rather than having this function to delete things, we just mark every single file we make with deleteOnExit(), that's generally what you do with temporary test files I believe. Regardless, I'm not going to block on it, just might be a better approach
| else { | ||
| Seq.empty[String] | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm not convinced that this is the right way to do this. Looking at the function, this adds some magical arguments from a file with a certain name, would it not be better to have the caller pass the filename to the function itself?
There was a problem hiding this comment.
@jackkoenig I agree this could be more elegant, but this works and there are things backed up behind this. The problem is one we have discussed about the bottom of the toolchain needing to pass stuff back up. Annotations are a candidate mechanism, there are probably others. In this case firrtl moves the files, because it knows about what's going on and there is stuff inline that it is well qualified to see. It does not know that these files are being placed there for some subset of the available backends, so the backend currently has to know that if such a file exists then it needs to add a command line options to use them.
There was a problem hiding this comment.
We can discuss improving in the future, I'm fine with merging this to get stuff working. Hopefully once we find a solution for how to pass data back "up" we can refactor this as well.
|
@jackkoenig can you change your review to approve then, please |
moving backend compilation utilities inadvertently lost the addition of a the black box verilog file list.
This fixes, that as well, as a couple of implementation bugs that slipped in. Motivated by chisel PR Move Backend Compilation Utilities