SystemVerilog: user defined type support by hirooih · Pull Request #2653 · universal-ctags/ctags

hirooih · 2020-09-27T14:11:04Z

@masatake san,
I've working on #1488 and #2413. It is not completed yet, but let me share the current status with you.

First I checked uncovered lines in verilog.c and add some test cases.
After that I made several refactoring changes.
Until now I made changes which did not affect on the output. Next I will work on variable/net type checking routines.

In addition to the Unit Tests I've been using source code of UVM 1.2 and an opensource RISC CPU to ensure output was not changed.

This is a draft pull request. Your feedback and advice are welcome.
Thank you.

…sk-function.d These covers some of uncovered lines. Incorrect in extected.tags due to known issues are marked as FIXME.

processTypedef: remove skipping {...} obsoleted by processStruct (cf. universal-ctags#2643) createTag(): divide covered/uncovered condition. findVerilogTags(): make more simple and robust mark uncovered lines as "FIXME: uncoverd"

remove isPrototype()

coveralls · 2020-09-27T14:15:19Z

Coverage increased (+0.03%) to 86.997% when pulling 924ef4e on hirooih:sv-type-refactoring into 86bdb84 on universal-ctags:master.

masatake · 2020-09-28T06:34:20Z

-	K_CONSTANT,
+	/* parser private items */
+	K_IGNORE = -16,
+	K_DEFINE,


s it the best way to tag "foo" of "`define foo ..." with constant kind?
Introducing "d/definition" kind and tagging the "foo" with the definition kind is an alternative way.

Using the unified kind "constant" in many areas reduces the information that client tools get from a tags file.
In my experience, it is better to tag different things with different kinds. If ctags hides the differences, and just reports various language objects as "constant," in some cases, a client tool must parse the raw .sv source file for distinguishing whether a given tag is "`define"'ed or not.

IMHO, ctags should not be too smart. Instead, ctags should be stupid; it should report what it sees in source input files as-is to client tools that will do something smart.

Sorry, what I wrote here is pointless; introducing the d/definition kind breaks compatibility.
So introducing the kind is not so easy.

You are right.
And I also agree with you that `define should have its own kind if we are allowed some incompatibility.

masatake · 2020-09-28T06:43:17Z

I reviewed up to 69e311f.

masatake · 2020-09-30T23:45:01Z

 *   FUNCTION DEFINITIONS
 */

+static void updateKind (tokenInfo *const token);


A minor issue.
updateKind here is not a function definition. So I would like you to introduce FUNCTION PROTOTYPES section.
See the following example taken from c.c:

/* * FUNCTION PROTOTYPES */ static void createTags (const unsigned int nestLevel, statementInfo *const parent); /* * FUNCTION DEFINITIONS */ /* * Token management */ static void initToken (tokenInfo* const token) { token->type = TOKEN_NONE;

masatake · 2020-10-01T00:07:50Z

 		dropEndContext (token);
 	}
+	if (token->kind == K_END_DE)
+		return;


My understanding is that this line is called only for broken (invalid syntax) input.
In that case, putting verbose() here may help us in the future.

This line is called for correct input, endfunction, endmodule, endtask, ..., etc.

Your comment reminds me one more question.
I think we should call notice() instead of verbose() for broken input.
And every parser should use same format something like this this.

How do you think?

I think notice() is too noisy. Furthermore, verbose is also noise for the purpose. Remember ctags will deal with large codebases.

Printing such information has two purposes.

reducing pointless bug reports
We know the motto, "if the input is broken, ctags may emit garbages". However, users may not know.
As the result, we have chances to receive bug reports of ctags to that broken input is given.
The notice can notify that broken input is given to ctags.

debugging
This is for us, not for users. For this purpose, verbose() and notice() are not suitable.
We can use macros in trace.h. See parsers/fypp.c.
The macros can be used only with ctags built with ./configure --enable-debugging.
--_trace= must be passed to ctags at runtime.

masatake · 2020-10-01T00:17:22Z

I reviewed up to "SystemVerilog: mark fork as K_BEGIN and join* as K_END to create a co…" (bc88a0c).

masatake · 2020-10-01T00:46:00Z

 }

+/* read an identifier, keyword, number, compiler directive, or macro identifier */
 static bool readIdentifier (tokenInfo *const token, int c)


How do you think about renaming this function to "readToken"?

I have been thinking exactly same thing.
I will rename them after this issue is complete.

masatake · 2020-10-01T00:47:55Z

+{
+	if (token->kind == K_UNDEFINED)
+	{
+		for (int i = 0; i < token->name->length; i++)


You can use vStringLength (token->name) instead of token->name->length..

masatake · 2020-10-01T00:49:15Z

+	{
+		for (int i = 0; i < token->name->length; i++)
+		{
+			int c = token->name->buffer[i];


vStringChar (token->name, i) can be used.

masatake · 2020-10-01T00:56:37Z

Mostly it looks good to me.
I pointed out some minor issues as a result of the review.

masatake · 2020-10-01T00:58:18Z

BTW, you can make an issue for tracking SystemVerilog issued as I did for Ruby in #2480.

hirooih · 2020-10-01T08:49:14Z

@masatake san,

Thank you for your review.
I'll fix the issues you pointed.

Which way do you prefer?

make one commit for these issue. You can see the difference from code you already reviewed.
squash each fix to the original commit. The change may not be clear for you.
make each commit. Squash them after you reviewed.

I prefer 1 or 2.
1 is easiest. 2 and 3 are almost same for me. This time changes are so trivial that you don't have to review seriously. 2 may be enough.
Anyway I will follow your way.

Let me ask you some questions.

Q1. "//" type comment vs "/* */" type comment
Many other parsers are using "//" style comment, but I found verilog.c has been using only "/* */" style..
I already use it in my local commits. Should I change them to "/* */" style comment?

Q2. nested if-else vs switch case in findTag()
As you see, the nested if-else in findTag() can be replaced with a switch case. I prefer switch case because it gives readers important information that all condition depend on one variable (token->kind). How do you think?

Supporting user-defined type is almost working well on my working directory. I am adding some more tests and make some refactoring. I hope I can commit it this weekend.

masatake · 2020-10-01T15:20:13Z

squash each fix to the original commit. The change may not be clear for you.

The original commits are readable. They are short enough so I can understand. Please, keep the way.
However, if you can reflect my review comment to the commit that I mentioned to easily, update the commit.
As I wrote before, git rebase -i may help you.

The original commits convince me you are more than a contributor.
About coding style, do as you want.

About indent-style, see .editorconfig. About C syntax, in most of the areas, we choose stdc99.
Many different people have joined the development of ctags (and left), so the style of coding is not unified.
Ideally, they should be unified. However, I have kept it as is. If a parser has no active maintainer, I adjust the
coding style of the parser as I like. If a parser has an active maintainer, generally, I say nothing; the maintainer
should do as one wants. And, finally, the SystemVerilog parser gets a new maintainer.

If you get a question about coding, asking me is one of the ways. Inspecting other parsers is an alternative way.

…ement()

…ame. refactor: define dropContext()

…ntext as begin-end does.

…GNORE This is required to distinguish an identifier from a keyword. define verilogKeywords[] and systemVerilogKeywords[]. systemverilog-net-var.d/expected.tags is changed because "packed" is ignored properly.

add interconnect, type, and void remove K_IGNORE reorder alphabetically

codecov · 2020-10-02T14:34:02Z

Codecov Report

Merging #2653 into master will increase coverage by 0.03%.
The diff coverage is 96.04%.

@@            Coverage Diff             @@
##           master    #2653      +/-   ##
==========================================
+ Coverage   86.86%   86.90%   +0.03%     
==========================================
  Files         183      183              
  Lines       38973    39016      +43     
==========================================
+ Hits        33855    33906      +51     
+ Misses       5118     5110       -8

Impacted Files	Coverage Δ
parsers/verilog.c	`98.79% <96.04%> (+1.58%)`	⬆️

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 86bdb84...924ef4e. Read the comment docs.

hirooih · 2020-10-02T14:37:50Z

squash each fix to the original commit. The change may not be clear for you.

I've done this.
The moving the prototype definition is squashed into 7bbbffb.
vString*() fixes were done by 'git --amend'.

I will do other additional refactoring after fixes which I am working will be done.

About coding style, do as you want.

OK. I'll do my best.

…rays.

Now it can support more kinds of delay format, universal-ctags#123, #foo, #`FOO, # (...), and etc.

masatake · 2020-10-02T20:09:45Z

I sent you an invitation to the u-ctags org. Please, merge your changes for yourself after accepting it.
There are some methods for merging in GitHub web UI. If there is no special requirement, I choose the "Merge pull request" method.

…s type

hirooih · 2020-10-04T09:41:52Z

Finally I've committed the fix for user defined type support.
a54e815 is the main fix.

By supporting user defined type correctly some expected.tags must be updated.
I also add some test cases.

masatake · 2020-10-04T09:48:49Z

I found a very minor issue.
About a54e815,
' in the second line of the commit log is not closed:

is followed by ',', ':', or ').
-----------------------------^

masatake · 2020-10-04T10:00:24Z

+			deleteToken (token);
 		}
 	}
+	vUngetc (c);


if c is not '#', the memory object associated with token leaks.

install valgrind

git checkout a54e815

make

make units LANGUAGES=SystemVerilog VG=1

cat Units/parser-verilog.r/systemverilog-class.d/VALGRIND.tmp

==20931== Memcheck, a memory error detector ==20931== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al. ==20931== Using Valgrind-3.14.0 and LibVEX; rerun with -h for copyright info ==20931== Command: ./ctags --verbose --options=NONE --optlib-dir=+./Units/parser-verilog.r/systemverilog-class.d/optlib -o - --options=./Units/parser-verilog.r/systemverilog-class.d/args.ctags ./Units/parser-verilog.r/systemverilog-class.d/input.sv ==20931== Parent PID: 19254 ==20931== ==20931== ==20931== HEAP SUMMARY: ==20931== in use at exit: 768 bytes in 21 blocks ==20931== total heap usage: 6,777 allocs, 6,756 frees, 360,392 bytes allocated ==20931== ==20931== 768 (264 direct, 504 indirect) bytes in 3 blocks are definitely lost in loss record 7 of 7 ==20931== at 0x4C2EE3B: malloc (vg_replace_malloc.c:309) ==20931== by 0x4149BD: eMalloc (routines.c:220) ==20931== by 0x471C8E: newToken (verilog.c:380) ==20931== by 0x472147: processParameterList (verilog.c:1235) ==20931== by 0x4736A8: processClass (verilog.c:1304) ==20931== by 0x4736A8: findTag (verilog.c:1577) ==20931== by 0x4736A8: findVerilogTags (verilog.c:1648) ==20931== by 0x411292: createTagsForFile (parse.c:3722) ==20931== by 0x411292: createTagsWithFallback1 (parse.c:3844) ==20931== by 0x4115E3: createTagsWithFallback (parse.c:3934) ==20931== by 0x4115E3: parseMio (parse.c:4096) ==20931== by 0x411746: parseFileWithMio (parse.c:4142) ==20931== by 0x411873: parseFile (parse.c:4079) ==20931== by 0x403307: createTagsForEntry (main.c:221) ==20931== by 0x40364F: createTagsForArgs (main.c:266) ==20931== by 0x40364F: batchMakeTags (main.c:360) ==20931== by 0x403AD5: runMainLoop (main.c:332) ==20931== by 0x403AD5: ctags_cli_main (main.c:585) ==20931== ==20931== LEAK SUMMARY: ==20931== definitely lost: 264 bytes in 3 blocks ==20931== indirectly lost: 504 bytes in 18 blocks ==20931== possibly lost: 0 bytes in 0 blocks ==20931== still reachable: 0 bytes in 0 blocks ==20931== suppressed: 0 bytes in 0 blocks ==20931== ==20931== For counts of detected and suppressed errors, rerun with: -v ==20931== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

The GitHub action "run units target under VALGRIND" looks broken. It should report this memory leak. I must fix it.

tagNameList: an identifier is classified as net or variable name when it is followed by ',', ':', or ')'. define processParameterList() for both module and class parameter list. It is based on the code which was included in processClass(). tags are output in the order defined. (class parameter was in reversed order.) parens in expression was handled by skipPastMatch(). type with dimenssion is supported better support of class inheritance only identifiers followed by ',', ':', or ')' are output as nets or variables labels are not passed to tagNameLlist() anymore.

inheritance is handled correctly parameters are output in order defined. add systemverilog-parameter.d/

add more tests: systemverilog-net-var.d update expected.tags for user defined type

hirooih · 2020-10-04T10:48:18Z

Thank you for your prompt reply.

make units LANGUAGES=SystemVerilog VG=1

This is great!

I've fix it and confirm VG=1 test passes.

masatake · 2020-10-05T02:53:40Z

BTW, I found an interesting document: https://docs.ctags.io/en/latest/contributions.html#
I cannot remember when I wrote this.

hirooih · 2020-10-05T13:38:31Z

Please, merge your changes for yourself after accepting it.

I missed this sentence. Thank you for your review and approval.

masatake · 2020-10-05T15:28:03Z

Thank you and welcome to the team.

masatake · 2020-10-05T15:29:15Z

Please close #1488 and #2413 if the issues are solved with the commits.

hirooih · 2020-10-05T23:46:09Z

The fixes in this pull request include the following user visible changes;

a fork-join creates a context as begin-end does
delays like #foo, #'foo, Added php patch #10, etc. are handled correctly
multi-dimensional array (ex. [1:0] [7:0])is supported
parameter assignment on function definition ignored properly
parameter list also support user defined types

hirooih added 5 commits September 24, 2020 23:21

Units,SystemVerilog: add systemverilog-net-var.d and systemverilog-ta…

cf9ae11

…sk-function.d These covers some of uncovered lines. Incorrect in extected.tags due to known issues are marked as FIXME.

SystemVerilog: fixes for uncovered lines

94d34aa

processTypedef: remove skipping {...} obsoleted by processStruct (cf. universal-ctags#2643) createTag(): divide covered/uncovered condition. findVerilogTags(): make more simple and robust mark uncovered lines as "FIXME: uncoverd"

SystemVerilog,refactor: combine vGetc () and skipWhite()

9195f02

SystemVerilog,refactor: define K_DEFINE and processDefine()

7bcee21

SystemVerilog,refactor: define "extern" and "pure" as K_PROTOTYPE

69e311f

remove isPrototype()

masatake self-requested a review September 27, 2020 22:53

masatake reviewed Sep 28, 2020

View reviewed changes

masatake reviewed Sep 30, 2020

View reviewed changes

masatake reviewed Oct 1, 2020

View reviewed changes

Comment thread parsers/verilog.c

masatake reviewed Oct 1, 2020

View reviewed changes

Comment thread parsers/verilog.c

hirooih added 9 commits October 2, 2020 23:18

SystemVerilog,refactor: define K_IFDEF and skipIfdef()

7bbbffb

SystemVerilog,refactor: define K_BEGIN and K_END

286499b

SystemVerilog,refactor: define K_END_DE

23bc0b7

SystemVerilog,refactor: define processAssertion() and processDesignEl…

0c7db87

…ement()

SystemVerilog: call findBlockName() in dropEndContext to read block n…

53ae1d4

…ame. refactor: define dropContext()

SystemVerilog: mark fork as K_BEGIN and join* as K_END to create a co…

1e1b1e9

…ntext as begin-end does.

SystemVerilog: mark unspecified Verilog/SystemVerilog keywords as K_I…

612ee93

…GNORE This is required to distinguish an identifier from a keyword. define verilogKeywords[] and systemVerilogKeywords[]. systemverilog-net-var.d/expected.tags is changed because "packed" is ignored properly.

SystemVerilog: update KeywordTable[]

0b0f125

add interconnect, type, and void remove K_IGNORE reorder alphabetically

SystemVerilog: mark `undef as K_IFDEF to skip a macro identifier.

efa8dd4

hirooih added 2 commits October 3, 2020 00:01

SystemVerilog: define skipDimension() to support multi-dimensional ar…

acdf181

…rays.

SystemVerilog: define skipDelay() to support various delay formats.

925a79d

Now it can support more kinds of delay format, universal-ctags#123, #foo, #`FOO, # (...), and etc.

masatake mentioned this pull request Oct 2, 2020

make test framework pass shellcheck lsof-org/lsof#110

Closed

masatake marked this pull request as ready for review October 2, 2020 22:02

masatake approved these changes Oct 2, 2020

View reviewed changes

SystemVerilog: processFunction(): skip parameter assignment of a clas…

9eac6db

…s type

hirooih force-pushed the sv-type-refactoring branch from 90f2ec4 to 560077b Compare October 4, 2020 09:37

masatake reviewed Oct 4, 2020

View reviewed changes

hirooih force-pushed the sv-type-refactoring branch from 560077b to 9a887a3 Compare October 4, 2020 10:43

hirooih added 4 commits October 4, 2020 19:44

SystemVerilog: skip the expression if ## operator is found

03df649

Units,SystemVerilog: update for processParameterList() fix.

ff06250

inheritance is handled correctly parameters are output in order defined. add systemverilog-parameter.d/

Units,SystemVerilog: updates for user defined type

924ef4e

add more tests: systemverilog-net-var.d update expected.tags for user defined type

hirooih force-pushed the sv-type-refactoring branch from 9a887a3 to 924ef4e Compare October 4, 2020 10:45

This was referenced Oct 4, 2020

[systemverilog] parser mistakes typedef for a module's port #2413

Closed

SystemVerilog: variables/class members of custom type not captured #1488

Closed

hirooih merged commit 9ce3543 into universal-ctags:master Oct 5, 2020

This was referenced Oct 5, 2020

SystemVerilog: wrong output on LRM and UVM-1.2 code #2618

Closed

SystemVerilog: no test case for e/event kind #2649

Closed

Conversation

hirooih commented Sep 27, 2020

Uh oh!

coveralls commented Sep 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

masatake commented Sep 28, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

masatake commented Oct 1, 2020

Uh oh!

Uh oh!

masatake Oct 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

masatake commented Oct 1, 2020

Uh oh!

masatake commented Oct 1, 2020

Uh oh!

hirooih commented Oct 1, 2020

Uh oh!

masatake commented Oct 1, 2020

Uh oh!

codecov bot commented Oct 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

hirooih commented Oct 2, 2020

Uh oh!

masatake commented Oct 2, 2020

Uh oh!

hirooih commented Oct 4, 2020

Uh oh!

masatake commented Oct 4, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hirooih commented Oct 4, 2020

Uh oh!

masatake commented Oct 5, 2020

Uh oh!

hirooih commented Oct 5, 2020

Uh oh!

masatake commented Oct 5, 2020

Uh oh!

masatake commented Oct 5, 2020

Uh oh!

hirooih commented Oct 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coveralls commented Sep 27, 2020 •

edited

Loading

masatake Oct 1, 2020 •

edited

Loading

codecov bot commented Oct 2, 2020 •

edited

Loading