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: refactoring changes #2664

Merged
merged 11 commits into from
Oct 11, 2020

Conversation

hirooih
Copy link
Contributor

@hirooih hirooih commented Oct 10, 2020

Before making fixes for known issues I made several refactoring changes.
See comment on each commit. The number of line of change is large, but each of them is simple change.

make units LANGUAGES=SystemVerilog,Verilog VG=1 passed.

readWordToken():
  clear token->name only when it read a word token.
  simplify the calculation of the return value
process*() can use "token" object freely by this fix.
instead of K_UNDEFINED for an identifier token

This makes the intension of source code more clear.
@coveralls
Copy link

coveralls commented Oct 10, 2020

Coverage Status

Coverage increased (+0.002%) to 87.009% when pulling 2884c2a on hirooih:sv-switch-refactoring into 952bd1e on universal-ctags:master.

@codecov
Copy link

codecov bot commented Oct 10, 2020

Codecov Report

Merging #2664 into master will increase coverage by 0.01%.
The diff coverage is 99.27%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2664      +/-   ##
==========================================
+ Coverage   86.90%   86.91%   +0.01%     
==========================================
  Files         183      183              
  Lines       39016    39014       -2     
==========================================
+ Hits        33906    33909       +3     
+ Misses       5110     5105       -5     
Impacted Files Coverage Δ
parsers/verilog.c 98.96% <99.27%> (+0.16%) ⬆️
main/parse.c 95.09% <0.00%> (-0.01%) ⬇️
main/vstring.h 100.00% <0.00%> (ø)
main/ptag.c 93.58% <0.00%> (+0.25%) ⬆️
main/options.c 83.63% <0.00%> (+0.26%) ⬆️

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 9ce3543...2884c2a. Read the comment docs.

@masatake masatake self-requested a review October 10, 2020 09:12
{
tagEntryInfo tag;
verilogKind kind;

Copy link
Member

Choose a reason for hiding this comment

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

(This is not a must.)
How about putting "Assert (kind >= 0);" here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point!

I left the following comment.
On some condition these lines are executed. I cannot assert here.
I need some more code clean-up.

	/* FIXME: This if-clause should be removed. */
	if (kind == K_UNDEFINED || kind == K_IDENTIFIER)
	{
		verbose ("Unexpected token kind %d\n", kind);
		return;
	}

@masatake
Copy link
Member

LGTM.

@masatake
Copy link
Member

You use verbose in some places. It is o.k. but I would like to introduce trace.h as an alterantive.

Macros defined in main/trace.h are useful. In many situations, they are better than verbose.
See parsers/jscript.c as examples.
Only one weakness of the macros is that you must prepare for enabling it.

make clean
./configure --enable-debugging
make
./ctags --_trace=<LANG> ...

The macros are developed for new C/C++ parsers.
Example output:

[jet@living]~/var/ctags% cat foo.c
int
main(void)
{
	return 0;
}
[jet@living]~/var/ctags% ./ctags --_trace=C ./foo.c
[>> parseFile][at 0] Parsing file ./foo.c
    [cxxCParserInitialize][at 0] Parser initialize for language C
    [>> cxxCParserMain][at 0] 
        [cxxKeywordEnablePublicProtectedPrivate][at 0] Disabling public/protected/private keywords
        [cppInitCommon][at 0] cppInit: brace format is 0
        [>> cxxParserParseBlockInternal][at 0] 
            [cxxParserParseBlockInternal][at 1] Token 'int' of type 0x04
            [cxxParserParseBlockInternal][at 2] Token 'main' of type 0x02
            [cxxParserParseBlockInternal][at 2] Token '(' of type 0x100000
            [>> cxxParserParseAndCondenseCurrentSubchain][at 2] 
                [>> cxxParserParseAndCondenseSubchainsUpToOneOf][at 2] Token types = 0x1000001(EOF ClosingParenthesis), reduce = 0
                [<< cxxParserParseAndCondenseSubchainsUpToOneOf][at 2] Got terminator token ')' 0x1000000
            [<< cxxParserParseAndCondenseCurrentSubchain][at 2] 
            [cxxParserParseBlockInternal][at 3] Token '{' of type 0x80000
            [>> cxxParserParseBlockHandleOpeningBracket][at 3] 
                [>> cxxParserExtractFunctionSignatureBeforeOpeningBracket][at 3] 
                    [cxxParserExtractFunctionSignatureBeforeOpeningBracket][at 3] Looking for function in 'int main(void) { '
                    [>> cxxParserLookForFunctionSignature][at 3] 
                        [cxxParserLookForFunctionSignature][at 3] Looking for function signature in 'int main(void) '
                        [cxxParserLookForFunctionSignature][at 3] Token 'int' of type 0x04 (Keyword)
                        [cxxParserLookForFunctionSignature][at 3] Not a parenthesis chain: assume this can be skipped
                        [cxxParserLookForFunctionSignature][at 3] Token 'main' of type 0x02 (Identifier)
                        [cxxParserLookForFunctionSignature][at 3] Not a parenthesis chain: assume this can be skipped
                        [cxxParserLookForFunctionSignature][at 3] Token '' of type 0x10000000 (ParenthesisChain)
                        [cxxParserLookForFunctionSignature][at 3] Found interesting parenthesis chain: check for identifier
                        [cxxParserLookForFunctionSignature][at 3] Got identifier before parenthesis chain
                        [>> cxxParserLookForFunctionSignatureCheckParenthesisAndIdentifier][at 3] 
                            [>> cxxParserTokenChainLooksLikeFunctionParameterList][at 3] 
                            [<< cxxParserTokenChainLooksLikeFunctionParameterList][at 3] Found closing parenthesis, it's OK
                        [<< cxxParserLookForFunctionSignatureCheckParenthesisAndIdentifier][at 3] Looks like valid parenthesis chain
                    [<< cxxParserLookForFunctionSignature][at 3] Found function signature
                    [>> cxxParserEmitFunctionTags][at 3] 
                        [cxxParserEmitFunctionTags][at 3] Scope start is 0, push scope is 1
                        [cxxParserEmitFunctionTags][at 3] Identifier is 'main'
                        [>> cxxTagCheckTypeField][at 3] 
                        [<< cxxTagCheckTypeField][at 3] 
                        [cxxTagCheckAndSetTypeField][at 3] Type name is 'int'
                        [cxxTagCommit][at 3] Emitting tag for symbol 'main', kind 'function', line 2
                        [cxxTagCommit][at 3] Tag has typeref typename int
                        [cxxParserEmitFunctionTags][at 3] Emitted function 'main'
                        [cxxScopePushTop][at 3] Pushed scope: 'main'
                    [<< cxxParserEmitFunctionTags][at 3] 
                [<< cxxParserExtractFunctionSignatureBeforeOpeningBracket][at 3] 
                [>> cxxParserParseBlockInternal][at 3] 
                    [cxxParserParseBlockInternal][at 4] Token 'return' of type 0x04
                    [>> cxxParserParseAndCondenseSubchainsUpToOneOf][at 4] Token types = 0x41(EOF Semicolon), reduce = 0
                    [<< cxxParserParseAndCondenseSubchainsUpToOneOf][at 4] Got terminator token ';' 0x40
                    [cxxParserParseBlockInternal][at 5] Token '}' of type 0x800000
                [<< cxxParserParseBlockInternal][at 5] Closing bracket!
                [cxxScopeTakeTop][at 5] Popped scope: ''
            [<< cxxParserParseBlockHandleOpeningBracket][at 5] 
        [<< cxxParserParseBlockInternal][at 5] EOF in main block
    [<< cxxCParserMain][at 5] 
[<< parseFile][at 5] 

ENTER/LEAVE macros adjust the depth of indentation well.

This is nothing to do with my reviewing for this pull request. Just for your information.

@hirooih hirooih force-pushed the sv-switch-refactoring branch from 45dc889 to 2884c2a Compare October 10, 2020 15:19
@hirooih
Copy link
Contributor Author

hirooih commented Oct 10, 2020

Thank you. I will study trace.h. It looks good.

After checking the coverage report, I made a fix on skipExpression() and I added test code.

LGTM.

May I commit this?

@masatake
Copy link
Member

masatake commented Oct 10, 2020

May I commit this?

Yes.

@hirooih
Copy link
Contributor Author

hirooih commented Oct 11, 2020

Thank you for your review.

@hirooih hirooih closed this Oct 11, 2020
@hirooih hirooih reopened this Oct 11, 2020
@hirooih hirooih merged commit 01b9fc8 into universal-ctags:master Oct 11, 2020
@hirooih
Copy link
Contributor Author

hirooih commented Oct 11, 2020

I did not merged this pull request.
I merged and close this request.

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