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

Remove usages of @chiselName, which is deprecated #2107

Closed
mwachs5 opened this issue Aug 31, 2021 · 15 comments
Closed

Remove usages of @chiselName, which is deprecated #2107

mwachs5 opened this issue Aug 31, 2021 · 15 comments
Labels
good first issue An issue whose fix is simple. Perfect for a new developer wanting to get involved!

Comments

@mwachs5
Copy link
Contributor

mwachs5 commented Aug 31, 2021

Type of issue: other enhancement

Impact: API modification | unknown

Development Phase: proposal

Other information

If the current behavior is a bug, please provide the steps to reproduce the problem:

What is the current behavior?

Currently there are many usages of @chiselName in the codebase, but this is deprecated and you get warnings when compiling.

What is the expected behavior?

We get no warnings related to using deprecated @chiselName.

Please tell us about your environment:
- version: master

What is the use case for changing the behavior?

Fewer warnings issued when compiling. Things that are deprecated should be removed.

@mwachs5 mwachs5 added the good first issue An issue whose fix is simple. Perfect for a new developer wanting to get involved! label Aug 31, 2021
@Burnleydev1
Copy link
Contributor

Hello @mwachs5 has this issue been solved?
If not please I wish to ask for more information on it.

@mwachs5
Copy link
Contributor Author

mwachs5 commented Oct 14, 2021

Hi @Burnleydev1 , no this issue has not been solved.

@chiselName is an older macro that improved the names of signals in the generated verilog. It is no longer necessary in 3.5 because we use a different technique( compiler plugin), and in fact you would see a bunch of warnings when compiling the scala code because every time you use @chiselName you get a deprecation warning. In order to clean up these errors in the code base, we would like to remove all the usages of @chiselName: https://github.com/chipsalliance/chisel3/search?q=chiselName

Note you will need to be a little careful which ones to delete, you only want to delete the usages, like this one:
https://github.com/chipsalliance/chisel3/blob/83eeda2efebb1e3e3d66365d388f5de627766d47/src/main/scala/chisel3/util/Counter.scala#L30

but not the tests of chiselName itself, like this one: https://github.com/chipsalliance/chisel3/blob/83eeda2efebb1e3e3d66365d388f5de627766d47/src/test/scala/chiselTests/NamingAnnotationTest.scala#L51

we will delete the tests when we fully delete @chiselName.

@mwachs5
Copy link
Contributor Author

mwachs5 commented Oct 14, 2021

Actually @chiselName is not technically deprecated, the warning using it causes that we would like to get rid of is:

[warn] /home/runner/work/chisel3/chisel3/src/main/scala/chisel3/util/Arbiter.scala:103:2: Auto-application to `()` is deprecated. Supply the empty argument list `()` explicitly to invoke method length,
[warn] or remove the empty argument list from its definition (Java-defined methods are exempt).
[warn] In Scala 3, an unapplied method like this will be eta-expanded into a function.
[warn] @chiselName

Removing the macro @chiselName should be fine for getting rid of this error message.

@Burnleydev1
Copy link
Contributor

Alright @mwachs5, I'll get on it now.

@mwachs5
Copy link
Contributor Author

mwachs5 commented Oct 14, 2021

For this one you may want to get your development environment set up, if you have not yet. Let me or @debs-sifive know if you need help with that!

@Burnleydev1
Copy link
Contributor

Please I need help with setting up an environment

@Burnleydev1
Copy link
Contributor

my development environment is set up but I must have done some things wrongly because it is not functioning normally.

@Burnleydev1
Copy link
Contributor

But if I'm getting you right then what you mean is that I should not delete the @chiselName in the test repository

@mwachs5
Copy link
Contributor Author

mwachs5 commented Oct 14, 2021

But if I'm getting you right then what you mean is that I should not delete the @chiselName in the test repository

there is a directory src/test/scala, some of the tests are specifically for @chiselName and there we shouldn't delete @chiselName. If there any others in that directory you could ask about them in your PR

@Burnleydev1
Copy link
Contributor

Sorry for asking many questions, but does this mean that in the src/main/scala directory I can remove all @chiselName?
or I should leave the ones that have the same or similar syntax as:
@chiselName
class OuterNamedNonModule {
val value = Wire(Bool())
}

@mwachs5
Copy link
Contributor Author

mwachs5 commented Oct 14, 2021

Yes, please delete all the ones in src/main/scala like the example you gave... also in core/src/main/scala

@mwachs5
Copy link
Contributor Author

mwachs5 commented Oct 14, 2021

and maybe once you delete those you can grep and give a listing of which still remain in your PR comments

@Burnleydev1
Copy link
Contributor

Okay thank you for the clarification @mwachs5.

@Burnleydev1
Copy link
Contributor

Hello @mwachs5, this is the link to the first pull request: #2186

@mwachs5
Copy link
Contributor Author

mwachs5 commented Feb 8, 2023

This was done in #2635

@mwachs5 mwachs5 closed this as completed Feb 8, 2023
jackkoenig pushed a commit that referenced this issue Feb 28, 2023
This PR adds a new annotation allowing inline loading for memory files
in Verilog code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue An issue whose fix is simple. Perfect for a new developer wanting to get involved!
Projects
None yet
Development

No branches or pull requests

2 participants