-
Notifications
You must be signed in to change notification settings - Fork 631
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: handle an identifier include '$' #2680
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2680 +/- ##
==========================================
- Coverage 86.93% 86.93% -0.01%
==========================================
Files 183 183
Lines 39080 39091 +11
==========================================
+ Hits 33974 33983 +9
- Misses 5106 5108 +2
Continue to review full report at Codecov.
|
All of uncovered lines increased are sanity checks for broken inputs. |
dup->inheritance = vStringNew (); | ||
vStringCopy (dup->name, token->name); | ||
vStringCopy (dup->blockName, token->blockName); | ||
vStringCopy (dup->inheritance, token->inheritance); |
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 does "delete", "new", and "copy" three vStrings.
However, when you call newToken () at the beginning of the funciton, the dup
returned from newToken() is already cleared.
So the "delete" and "new" steps are just redundant. What you need is only the the "copy" step.
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 review.
First I did make same mistake as you wrote. But it does not work.
dup->name
, dup->blockName
, and dup->inheritance
are overwritten by token->name
, token->blockName
, and token->inheritance
via *dup = *token;
.
vStrings allocated by dupToken()
are never be freed.
Another solution is copying each element without using *dup = *token;
. But I did not choose it because it might cause missing elements. I am assuming the number of vString elements are much less than the number of other elements.
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 the following code?
static tokenInfo *dupToken (tokenInfo *token)
{
tokenInfo *dup = newToken ();
{
tokenInfo tmp = *dup;
*dup = *token;
dup->name = tmp.name;
dup->blockName = tmp.blockName;
dup->inheritance = tmp.inheritance;
}
vStringCopy (dup->name, token->name);
vStringCopy (dup->blockName, token->blockName);
vStringCopy (dup->inheritance, token->inheritance);
return dup;
}
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.
Thanks. Better than mine.
I've committed the following code.
static tokenInfo *dupToken (tokenInfo *token)
{
tokenInfo *dup = newToken ();
tokenInfo tmp = *dup; // save vStrings, name, blockName, and inheritance
*dup = *token;
// revert vStrings allocated for dup
dup->name = tmp.name;
dup->blockName = tmp.blockName;
dup->inheritance = tmp.inheritance;
// copy contents of vStrings
vStringCopy (dup->name, token->name);
vStringCopy (dup->blockName, token->blockName);
vStringCopy (dup->inheritance, token->inheritance);
return dup;
}
readWordToken() calls clearToken() to ensure all fields are updated. The idea of dupToken() is contributed by masatake-san
They are tests for issue universal-ctags#624 caused by broken inputs. Outputs are undefined and changed by the fix on b4364b52.
Two 'FIXME' errors are fixed. Add continuous assignments tests. add test for identifiers '$' included
a275ebd
to
4219047
Compare
isIdentifierCharacter()
did not handle '$` properly.I needed to simplify token handling for the better fix of it.
Delay handling is also improved.