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

Fix scope resolution operator working with module/interface (#4890) #4906

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

ba-sc
Copy link

@ba-sc ba-sc commented Feb 19, 2024

Closes #4890

@@ -621,6 +621,11 @@ void V3ParseImp::tokenPipelineSym() {
} else {
token = yaID__ETC;
}
} else if (token == yaID__CC) {
if (VN_IS(scp, Module) || VN_IS(scp, Iface)) {
yylval.fl->v3error(scp->typeName() << " cannot be the left operand of a "
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
yylval.fl->v3error(scp->typeName() << " cannot be the left operand of a "
yylval.fl->v3error(scp->prettyTypeName() << " cannot be the left operand of a "

@chykon
Copy link
Contributor

chykon commented Feb 20, 2024

There is a problem with the t_class_mod_bad test: since the check for an invalid left operand of the scope resolution operator now happens earlier, the new error message overrides the expected old message.

Should I update the expected output for this test? Or do I need to come up with some other solution?

Also need to think about what to do with the existing check code:

verilator/src/V3LinkDot.cpp

Lines 3631 to 3640 in a951446

nodep->classOrPackagep(cpackagerefp->classOrPackagep());
if (!VN_IS(nodep->classOrPackagep(), Class)
&& !VN_IS(nodep->classOrPackagep(), Package)) {
cpackagerefp->v3error(
"'::' expected to reference a class/package but referenced '"
<< (nodep->classOrPackagep() ? nodep->classOrPackagep()->prettyTypeName()
: "<unresolved-object>")
<< "'\n"
<< cpackagerefp->warnMore() + "... Suggest '.' instead of '::'");
}

Perhaps it should be improved further? On the other side, the new code allows you to throw an error earlier.

@wsnyder
Copy link
Member

wsnyder commented Feb 20, 2024

Maybe have the test mis-reference a variable? The intent is to give code coverage to that (older) message - so whatever's needed to make it fire. (Unless it's impossible which I don't think on quick reading.)

@wsnyder
Copy link
Member

wsnyder commented Apr 30, 2024

@chykon do you think you'll be able to get back to finishing this pull request?

@chykon
Copy link
Contributor

chykon commented Apr 30, 2024

I can't for now, but if someone wants to continue working on PR, I don't mind.

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.

Getting a module parameter using scope resolution operator
3 participants