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

SystemVerilog: improve compiler directive support #2675

Merged
merged 2 commits into from
Oct 27, 2020

Conversation

hirooih
Copy link
Contributor

@hirooih hirooih commented Oct 25, 2020

All standard compiler directives are supported.
Only `define directive emits a tag. All other directives skip the whole line included.
The newline character preceded by a backslash is ignored to support a multi-line macro definition.

new tests are added.

@coveralls
Copy link

coveralls commented Oct 25, 2020

Coverage Status

Coverage increased (+0.002%) to 87.027% when pulling df4e705 on hirooih:sv-directive into 5d000b1 on universal-ctags:master.

@codecov
Copy link

codecov bot commented Oct 25, 2020

Codecov Report

Merging #2675 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2675   +/-   ##
=======================================
  Coverage   86.93%   86.93%           
=======================================
  Files         183      183           
  Lines       39079    39080    +1     
=======================================
+ Hits        33972    33974    +2     
+ Misses       5107     5106    -1     
Impacted Files Coverage Δ
parsers/verilog.c 99.32% <100.00%> (+0.16%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dfa2ebf...df4e705. Read the comment docs.

All standard compiler directives are supported.
Only `define directive emits a tag.  All other directives skip the whole line included.
The newline character preceded by a backslash is ignored to support a multi-line macro definition.
@hirooih
Copy link
Contributor Author

hirooih commented Oct 25, 2020

I checked uncovered lines (L704-709).

I tried to add testcases for this block in this pull-request, but I found that skipExprssion() introduced by 702c822 and K_DIRECTIVE condition block introduced by this pull-request made it uncovered.

I've added a test-case which covers the lines.

@masatake
Copy link
Member

I think it is nice to have a case for testing a multiline macro.
Other than this, the change looks good for me.

By the way, I recommend you to explore how our C parser support macros.

$ cat /tmp/foo.c

#define ARG_START  (
#define ARG_END    )
#define BEGIN   {
#define END     }
#define R(X)    return X
#define ZERO    R(0)
#define SEP     ;

int
main ARG_START
ARG_END
BEGIN
ZERO SEP
END

$ ./ctags -o - /tmp/foo.c
ARG_END	/tmp/foo.c	/^#define ARG_END /;"	d	file:
ARG_START	/tmp/foo.c	/^#define ARG_START /;"	d	file:
BEGIN	/tmp/foo.c	/^#define BEGIN /;"	d	file:
END	/tmp/foo.c	/^#define END /;"	d	file:
R	/tmp/foo.c	/^#define R(/;"	d	file:
SEP	/tmp/foo.c	/^#define SEP /;"	d	file:
ZERO	/tmp/foo.c	/^#define ZERO /;"	d	file:

$ ./ctags --param-CPreProcessor:_expand=true --fields-C=+'{macrodef}' --fields=+'{signature}' -o - /tmp/foo.c
ARG_END	/tmp/foo.c	/^#define ARG_END /;"	d	file:	macrodef:)
ARG_START	/tmp/foo.c	/^#define ARG_START /;"	d	file:	macrodef:(
BEGIN	/tmp/foo.c	/^#define BEGIN /;"	d	file:	macrodef:{
END	/tmp/foo.c	/^#define END /;"	d	file:	macrodef:}
R	/tmp/foo.c	/^#define R(/;"	d	file:	signature:(X)	macrodef:return X
SEP	/tmp/foo.c	/^#define SEP /;"	d	file:	macrodef:;
ZERO	/tmp/foo.c	/^#define ZERO /;"	d	file:	macrodef:R(0)
main	/tmp/foo.c	/^main ARG_START$/;"	f	typeref:typename:int	signature:()

In the latter invocation, main is captured as a function:-)
You may don't want to expand macros in your parser. However, extracting signatures of macros is not a bad idea:

R	/tmp/foo.c	/^#define R(/;"	d	file:	signature:(X)	macrodef:return X

@hirooih hirooih merged commit 4b7b8a2 into universal-ctags:master Oct 27, 2020
@hirooih
Copy link
Contributor Author

hirooih commented Oct 27, 2020

I think it is nice to have a case for testing a multiline macro.

Me, too. I've added it.

Other than this, the change looks good for me.

Thanks. I've merged this request.

You may don't want to expand macros in your parser.

No, I don't. If we need to expand macros, I prefer to use external preprocessor and use __LINE__ and __FILE__ directive.

However, extracting signatures of macros is not a bad idea:

Good suggestion. I will add them on the TODO list.

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