-
Notifications
You must be signed in to change notification settings - Fork 620
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
verilog: keep trace with instance in TAGS file #2703
Conversation
The way to write test cases is wrong. expected.tags is missing. See https://docs.ctags.io/en/latest/units.html. |
Thanks for enhancement.
|
And;
|
Thanks for your work on this awesome tool and advice. |
parsers/verilog.c
Outdated
{ | ||
if(c == '('){ |
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.
we don't use this style, putting {
at the end of line IN THIS FILE.
if (c == '(')
{
...
is expected.
Codecov Report
@@ Coverage Diff @@
## master #2703 +/- ##
=======================================
Coverage 86.99% 86.99%
=======================================
Files 185 185
Lines 39462 39465 +3
=======================================
+ Hits 34330 34334 +4
+ Misses 5132 5131 -1
Continue to review full report at Codecov.
|
parsers/verilog.c
Outdated
token->kind = K_UNDEFINED; | ||
kind = K_UNDEFINED; | ||
} | ||
else { |
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.
we don't use this style, putting { at the end of line IN THIS FILE.
else
{
...
is expected.
parsers/verilog.c
Outdated
kind = K_UNDEFINED; | ||
} | ||
else { | ||
verbose("find instance: %s with kind %d\n", vStringValue (token->name), token->kind); |
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.
How about printing the kind in a string?
verbose("find instance: %s with kind %s\n", vStringValue (token->name),getNameForKind(token->kind));
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.
(I myself didn't test the change that I showed.)
I would like you to squash the commits into one commit. In addition, I would like you to use "SystemVerilog" or "Verilog,SystemVerilog" instead of "[verilog/SystemVerilog]" as the prefix of the commit header like:
|
parsers/verilog.c
Outdated
token->kind = K_INSTANCE; | ||
kind = K_INSTANCE; | ||
c = skipPastMatch ("()"); | ||
if(c != ';' && c != ',') |
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.
We can simplify the code a bit like:
if (c == '(')
{
kind = K_UNDEFINED;
c = skipPastMatch ("()");
if (c == ';' || c == ',')
{
kind = K_INSTANCE;
verbose("find instance: %s with kind %s\n", vStringValue (token->name), getNameForKind(kind));
}
token->kind = kind;
}
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.
I will check this code in detail in this week.
Quick comments.
I propose the followings;
if (c == '(')
{
c = skipPastMatch ("()");
if (c == ';' || c == ',')
{
kind = K_INSTANCE;
verbose("find instance: %s with kind %s\n", vStringValue (token->name), getNameForKind(kind));
}
else // comment why this condition is required
kind = K_UNDEFINED;
token->kind = kind;
}
Actually I don't understand why the condition is required.
From LRM 23.3.2 Module instantiation syntax
module_instantiation ::= // from A.4.1.1
module_identifier [ parameter_value_assignment ] hierarchical_instance { , hierarchical_instance };
...
hierarchical_instance ::= name_of_instance ( [ list_of_port_connections ] )
name_of_instance ::= instance_identifier { unpacked_dimension }
...
Please take care of { unpacked_dimension }
part.
Please check interface instantiation syntax, too.
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.
From the units test, the parser will treat "`add_t" as instance if there isn't a "else" condition
var `add_t(foo) = '0;
I will check unpacked_dimension and interface syntax later, thanks.
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.
Thank you for your proposal.
Instance name support is very important. I completely missed the issue.
Please see my comments.
There is one issue. If I understand your fix correctly, it tags a void function as a instance.
// LRM 13.4.1 Return values and void functions
myprint(a); // call myprint (defined below) as a statement
function void myprint (int a); // FIXME
...
endfunction
Void function is not used often. Supporting instance tag is much more important. Leave it as known bug for now.
Please add the test case above, too.
Thanks.
parsers/verilog.c
Outdated
@@ -1577,8 +1580,26 @@ static int tagNameList (tokenInfo* token, int c) | |||
{ | |||
c = processType(token, c, &kind); // update token and kind | |||
|
|||
if (c == '=' || c == ',' || c == ';' || c == ')' || c == '`') | |||
if (c == '=' || c == ',' || c == ';' || c == ')' || c == '`' || c == '(' || c == '[') |
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.
Instance processing is dependent from variable processing. The following structure is more readable.
if (c == '=' || c == ',' || c == ';' || c == ')' || c == '`')
{
...
}
else if (c == '(' ...)
...
parsers/verilog.c
Outdated
{ | ||
while(c == '[') | ||
{ | ||
c = skipPastMatch ("[]"); |
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.
You can use skipDimension()
.
parsers/verilog.c
Outdated
} | ||
if(c == '(') | ||
{ | ||
c = skipPastMatch ("()"); |
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.
We need c= skipWhite (c);
here.
m inst () ; // a space before semicolon
fails. (You may want to add a test case for this. )
skipPastMatch()
should do the job, but it does not now. I will work on this issue on this weekend.
You may wait for the fix.
parsers/verilog.c
Outdated
@@ -127,7 +128,8 @@ static kindDefinition VerilogKinds [] = { | |||
{ true, 'p', "port", "ports" }, | |||
{ true, 'r', "register", "variable data types" }, | |||
{ true, 't', "task", "tasks" }, | |||
{ true, 'b', "block", "blocks (begin, fork)" } | |||
{ true, 'b', "block", "blocks (begin, fork)" }, | |||
{ true, 'i', "instance", "instance(module instance)" } |
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.
Please be consistent with the line above. i.e. "instance (module, instance)"
parsers/verilog.c
Outdated
@@ -140,6 +142,7 @@ static kindDefinition SystemVerilogKinds [] = { | |||
{ true, 'r', "register", "variable data types" }, | |||
{ true, 't', "task", "tasks" }, | |||
{ true, 'b', "block", "blocks (begin, fork)" }, | |||
{ true, 'i', "instance" "instance(module or interface instance)"}, |
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.
ditto
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.
Sorry for many change requests...
parsers/verilog.c
Outdated
@@ -1577,13 +1580,32 @@ static int tagNameList (tokenInfo* token, int c) | |||
{ | |||
c = processType(token, c, &kind); // update token and kind | |||
|
|||
if (c == '=' || c == ',' || c == ';' || c == ')' || c == '`') | |||
if (c == '=' || c == ',' || c == ';' || c == ')' || c == '`' ) |
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.
Remove a space you added.
parsers/verilog.c
Outdated
createTag (token, kind); | ||
} | ||
else | ||
{ |
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 else clause is not necessary.
parsers/verilog.c
Outdated
else if (c == '(' || c == '[') // should be instance? TODO: function void foo(); | ||
{ | ||
c = skipDimension(c); | ||
if(c == '(') |
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 if-clause is not necessary.
parsers/verilog.c
Outdated
/* c= skipWhite (c); */ | ||
if(c == ';' || c == ',') | ||
{ | ||
kind = K_INSTANCE; |
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 variable is not necessary.
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.
OK, I'm sorry for my poor coding skill 😭
parsers/verilog.c
Outdated
else if (c == '(' || c == '[') // should be instance? TODO: function void foo(); | ||
{ | ||
c = skipDimension(c); | ||
if(c == '(') |
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.
add a space after 'if'.
parsers/verilog.c
Outdated
@@ -127,7 +128,8 @@ static kindDefinition VerilogKinds [] = { | |||
{ true, 'p', "port", "ports" }, | |||
{ true, 'r', "register", "variable data types" }, | |||
{ true, 't', "task", "tasks" }, | |||
{ true, 'b', "block", "blocks (begin, fork)" } | |||
{ true, 'b', "block", "blocks (begin, fork)" }, | |||
{ true, 'i', "instance", "instances" } |
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.
If you chose "instances of module or interface" below, this should be "instances of module".
parsers/verilog.c
Outdated
@@ -140,6 +142,7 @@ static kindDefinition SystemVerilogKinds [] = { | |||
{ true, 'r', "register", "variable data types" }, | |||
{ true, 't', "task", "tasks" }, | |||
{ true, 'b', "block", "blocks (begin, fork)" }, | |||
{ true, 'i', "instance" "instances of module or interface"}, |
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.
Add a space before "}".
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.
It is getting simpler, isn't it?
c = skipDimension(c); | ||
c = skipPastMatch ("()"); | ||
c = skipWhite (c); | ||
if (c == ';' || c == ',') |
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.
Leave a comment here as we discussed.
parsers/verilog.c
Outdated
c = skipWhite (c); | ||
if (c == ';' || c == ',') | ||
{ | ||
kind = K_INSTANCE; |
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.
It seems that you missed my review comment this morning.
This variable is not necessary. You can specify K_INSTANCE directly.
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.
Get it, thanks!
parsers/verilog.c
Outdated
{ | ||
c = skipDimension(c); | ||
c = skipPastMatch ("()"); | ||
c = skipWhite (c); |
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.
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.
See my comment.
Again add a test case which fails by this issue. (Add a space in ");" at the end of one of module instantiations.)
For example;
verilog-instance.d/input.v: L57
.b (b));
Change this line as;
.b (b)) ; // spaces before semicolon
Remove;
c = skipWhite (c);
and confirm that the test fails.
And #2718 is merged into universal-ctags:master
.
Merge it into your branch and confirm the test passes.
parsers/verilog.c
Outdated
// var `add_t(foo) = '0; | ||
if (c == ';' || c == ',') | ||
{ | ||
verbose("find instance: %s with kind %s\n", vStringValue (token->name), getNameForKind(kind)); |
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.
kind
is in this line, too.
One more thing. Your fix made the following lines unnecessary.
Please remove them. Your fix added very small number of lines and gave us great functionality. It sounds great! |
SystemVerilog: give K_INSTANCE to createTag directly SystemVerilog: new test case Add a space in ");" at the end of one of module instantiationsadd
Again, thank you for your contribution. |
Thanks for your patience! |
I'm working on VHDL and I have studied some concepts of this technology area.
for uut7, ctags with this pull request emits:
I think we can fill typeref: field for representing the relationship between uut7 and foo.
I'm taking the same approach in VHDL. |
Thanks. |
add a kind named K_INST
update function tagNameList to update kind from K_IDENTIFIER to K_INST