-
Notifications
You must be signed in to change notification settings - Fork 630
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
(System-)Verilog: Parameters/Localparams/Specparams should be seperated #2537
Comments
Do you mean we should introduce new kinds for localparams and specparams?
I would like to know the name and descriptions for the new kinds we should add. |
Another approach is adding parser specific fields like C parser does:
The c parser doesn't introduce I'm not sure which is better (adding new kinds or storing the extra information to fields). |
I very much like the property proposal from @masatake, as it would alo preserve backward compatibility. This should also be done for defines as well, as they are preprocessor commands and treated very much differently when it comes to compileing/elbaortaing the desing. |
Could you show me how the tags file should be? |
I would propose exactly what you suggested from C.
I think that would be excellent and should be backwards compatible. |
I wonder why it is not "property". Is "properties" better than "property"? With patch:
With patch ctags emits:
|
You would be correct. definedWith is probably more appropriate. The output of the patch looks good to me. |
@masatake san, Shall I take over this issue? You already made the fix above. But it is not merged yet. |
Yes, please.
Your idea may be better. |
@hirooih, do you think adding the field is better than introducing more kinds? You know well about Verilog. So you can take a more aggressive approach. If you have a favorite editor plugin for editing or reading (System)Verilog code with tags file, talking with its developers is also good idea. |
If we could break backward compatibility, I would add new kind for define-macro ("d", "macro definitions" as c-parser), and add a field for parameter, localparam, and specparam as you proposed. define-macro is a part of pre-processor like C. But I also understand you have a strict rule, " do not to break compatibility". I think it is very important. I don't know any good ctags plugin designed for (System)Verilog. I am using ctags extension (plugin) designed for C on Visual Studio Code. It works well with Verilog extension. |
In my view, for verilog, breaking compatibility would be acceptable, for one simple reason: The problem originated from the fact, that e.g. an instantiation for modules would pick up localparams or specparams next to the regular module parameters, without a good way to tell to which scope they belong. |
Changing the kinds of (System)Verilog similar to the kinds of C is not a good idea if the motivation of change is to make the tags output for (System)Verilog input acceptable to the VSCode extension for C language. I would like you to imagine an ideal VSCode extension for (System)Verilog. @hirooih, if you don't know pseudo tags (ptag), I would like you to know it. We can introduce Consider the current version of the kinds of SystemVerilog is 1.0. Well-written client tools may read the version number of the parser and can handle the incompatiblities well. If you would like to try the ptags, let me know. I will implement them. BTW, I found the kind names are used in |
I'm using this extension. It is a great Verilog extension but its ctags support is useless. It execs ctags on each editing file (at L123). It cannot find symbols defined in other files. So I am using ctags extension (plugin) designed for C. |
I see. |
The VS code extension mentioned is exactly the one I am using, and where this ticket originates from. Using the ctag kinds, it is currently not possible to implement certain functions correctly, like the mentioned seperation of localparams and parameters for instantiating a module. This is the downstream ticket: |
I implemented
If a parser doesn't specify its version, the value for the ptag of the parser is "0.0.0". Client tools deal with the ptag. It will take costs of development. However, it will be better than introducing incompatible change silently. |
@masatake san I don't think the current definition of Verilog parser is so bad that we have to introduce new specification. We will have to maintain two versions (or more...) Adding a property as you proposed solves this paramter-issue. Let me confirm your request.
All of them are constants defined in LRM 6.20 Constants as
The value of parameter can be overridden directly in each instance. The value of localparam can not be overridden directly in each instance, but can be overridden indirectly via parameter values as in your example above.
I understand what you require is a list of parameters for a module instantiation. Let me explain what I mean. (It is more complicated than I thought first...) From LRM 6.20.1 Parameter declaration syntax
In the example below, you need P1, P2, P3, and P.4. You don't need L1, L3, L5, although they are defined by parameter.
If you have any other problem, let me know. |
It is not about 'needing' them, it is, as you pointed out, simply not possible to set a localparam in a module instantiation, as it is not visible to the outside (hence LOCALparam). Currently there is no way to split these from ctags output, which makes it very inconvenient if not impossible to autogenerate instances with parameter lists. specparams are once again a different topic, but they are used more rarely in day-to-day development, they usually |
I see. |
I've sent a pull-request of the fix for this issue on #2666. |
SystemVerilog: support property:parameter (update for #2537)
Finally the pull request is merged. Choosing the name of the new field was the most difficult problem. By default this feature is disabled. Add
Again if you have any other issue or request, please open new issue. |
The name of the parser: Systemverilog/Verilog
The command line you used to run ctags:
The content of input file:
The tags output you are not satisfied with:
The tags output you expect:
parameters, localparams and specparams have their own groups, because they differ significantly in terms of semantics. (e.g. a localparam is not totally 'constant' as it may depend on a parameter passed on instantiation). Currently they are just 'constants'.
The biggest practical problem though is that ctags is used by many editor plugins to enable
auto instantiation of verilog modules. But in an instance declaration, only parameters can be used, localparams and specparams are local to the scope of the module internals and cannot be set externally.
The current 'workaround' is to actually parse the line contents, but this is error prone because parameter must not be the first thing in the line when it is multi-dimensional and it may also be contained inside a variable name that follows. So a simple 'contains' check is by far not enough. And writing a line parser on top sort of defeats the prupose of ctags imho.
The version of ctags:
How do you get ctags binary:
macOSx homebrew
The text was updated successfully, but these errors were encountered: