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

Verilog: all text macro map to new kindDefinition:define #3653

Merged
merged 1 commit into from
Mar 2, 2023
Merged

Verilog: all text macro map to new kindDefinition:define #3653

merged 1 commit into from
Mar 2, 2023

Conversation

my2817
Copy link
Contributor

@my2817 my2817 commented Mar 1, 2023

In some case, readtags return different kind of text macro(define) and parameter is better for me.

@my2817 my2817 changed the title Verilog: all text macro map to new kindDefinition:define WIP:Verilog: all text macro map to new kindDefinition:define Mar 1, 2023
@my2817 my2817 changed the title WIP:Verilog: all text macro map to new kindDefinition:define Verilog: all text macro map to new kindDefinition:define Mar 1, 2023
@masatake
Copy link
Member

masatake commented Mar 1, 2023

You added a new kind. So we should update the parser-specific version number.

Could you merge the following change to your pull request?

diff --git a/man/ctags-lang-verilog.7.rst.in b/man/ctags-lang-verilog.7.rst.in
index 3e2362a98..ad8d36c05 100644
--- a/man/ctags-lang-verilog.7.rst.in
+++ b/man/ctags-lang-verilog.7.rst.in
@@ -195,6 +195,14 @@ KNOWN ISSUES
 
 See https://github.com/universal-ctags/ctags/issues/2674 for more information.
 
+VERSIONS
+--------
+
+Change since "0.0"
+~~~~~~~~~~~~~~~~~~
+
+* New kind ``macro``
+
 SEE ALSO
 --------
 
diff --git a/parsers/verilog.c b/parsers/verilog.c
index 4ba355146..044d1aeab 100644
--- a/parsers/verilog.c
+++ b/parsers/verilog.c
@@ -2086,6 +2086,8 @@ extern parserDefinition* VerilogParser (void)
 {
        static const char *const extensions [] = { "v", NULL };
        parserDefinition* def = parserNew ("Verilog");
+       def->versionCurrent = 1;
+       def->versionAge = 1;
        def->kindTable  = VerilogKinds;
        def->kindCount  = ARRAY_SIZE (VerilogKinds);
        def->fieldTable = VerilogFields;
@@ -2100,6 +2102,8 @@ extern parserDefinition* SystemVerilogParser (void)
 {
        static const char *const extensions [] = { "sv", "svh", "svi", NULL };
        parserDefinition* def = parserNew ("SystemVerilog");
+       def->versionCurrent = 1;
+       def->versionAge = 1;
        def->kindTable  = SystemVerilogKinds;
        def->kindCount  = ARRAY_SIZE (SystemVerilogKinds);
        def->fieldTable = SystemVerilogFields;

One more.
Could you update the list of kinds in man/ctags-lang-verilog.7.rst.in?

Maybe squashing the commits into one is better.

@masatake masatake requested a review from hirooih March 1, 2023 13:06
Copy link
Contributor

@hirooih hirooih left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think it is "better" to distinguish a define from constants.
To begin with, a define is not always a constant.

But the question is whether it is worth consuming the precious resource 'd'. I judged that it was not worth it.

Do you have any good reasons (or excuses:-))?

parsers/verilog.c Outdated Show resolved Hide resolved
parsers/verilog.c Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 2, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change

Comparison is base (7c84355) 82.82% compared to head (6c11e06) 82.82%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3653   +/-   ##
=======================================
  Coverage   82.82%   82.82%           
=======================================
  Files         223      223           
  Lines       54502    54506    +4     
=======================================
+ Hits        45143    45147    +4     
  Misses       9359     9359           
Impacted Files Coverage Δ
parsers/verilog.c 98.44% <100.00%> (+<0.01%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@masatake
Copy link
Member

masatake commented Mar 2, 2023

@my2817, the man is not updated.

@hirooih
Copy link
Contributor

hirooih commented Mar 2, 2023

@masatake san,

@my2817, the man is not updated.

I am a little confused.

All CI tests on GitHub had passed, so I merge this PR.
However, distcheck in the CircleCI builds is failing.


We have make check and make dist, but there is no make distcheck in .github/workflow/*. I think it was included before.
We should add it again.

@masatake
Copy link
Member

masatake commented Mar 3, 2023

The test cases on CircleCI are run for each pull request like #3643.

So the checks were included already. However, it was skipped in this pull request.
I wonder why...

I found some log messages in CircleCI side:

Could not find a usable config.yml, you may have revoked the CircleCI OAuth app.
Please sign out of CircleCI and log back in with your VCS before triggering a new pipeline.

It seems that the actions on CirclCI is recovered now; see the newer pull request: #3654.

@my2817 my2817 deleted the verilog-define-kind branch March 3, 2023 00:49
@my2817
Copy link
Contributor Author

my2817 commented Mar 3, 2023

@my2817, the man is not updated.

The following patch? We use precious resource "K_DEFINE", but not define a new K_MACRO...
OK, got it. "define" is a new kind to tags file. I will make a new PR, and you can merge or just close it by your judgement.

Thanks.

diff --git a/man/ctags-lang-verilog.7.rst.in b/man/ctags-lang-verilog.7.rst.in
index 3e2362a98..ad8d36c05 100644
--- a/man/ctags-lang-verilog.7.rst.in
+++ b/man/ctags-lang-verilog.7.rst.in
@@ -195,6 +195,14 @@ KNOWN ISSUES
 
 See https://github.com/universal-ctags/ctags/issues/2674 for more information.
 
+VERSIONS
+--------
+
+Change since "0.0"
+~~~~~~~~~~~~~~~~~~
+
+* New kind ``macro``
+
 SEE ALSO
 --------

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants