-
-
Notifications
You must be signed in to change notification settings - Fork 925
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
[Truffle] Module#deprecate_constant #4179
Conversation
return constant; | ||
} | ||
|
||
@TruffleBoundary | ||
private void warnDeprecatedConstant(String name) { | ||
getContext().getJRubyRuntime().getWarnings().warn("constant " + name + " is deprecated"); |
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.
This warning does not appear to be writing to the $stderr
that the spec is expecting.
One idea I had was to use a snippet node to call warn()
with a warning message. Is this an okay idea? Or is there a better way to fix this?
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.
A SnippetNode would be nice, but it needs the frame which might be inconvenient to pass in places.
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.
In this case a simple CallDispatchNode should be sufficient.
@TruffleBoundary | ||
private void warnDeprecatedConstant(String name) { | ||
getContext().getJRubyRuntime().getWarnings().warn("constant " + name + " is deprecated"); | ||
} |
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.
This is repeated in multiple nodes. Should this be consolidated somewhere?
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.
Yes, please make it a node.
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.
Maybe a WarnNode
that allows varargs arguments so it handle the String concatenation nicely on its side.
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.
I've added a warn node in the latest commit
@@ -49,6 +50,9 @@ protected RubyConstant lookupConstant(VirtualFrame frame, | |||
if (!isVisible) { | |||
throw new RaiseException(coreExceptions().nameErrorPrivateConstant(getModule(), name, this)); | |||
} | |||
if(constant != null && constant.isDeprecated()){ | |||
warnDeprecatedConstant(name); | |||
} |
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.
Should anything be done here performance wise? A binary profile? Or something else?
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.
No, since constant is @CompilationFinal
as it is @Cached
.
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.
Interesting, there is no uncached Specialization in this node, that is a bug.
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.
Added it in ea0c454
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.
Please add a check also on the uncached specialization when you rebase.
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.
I've added this check in the specialization you've added in the latest commit
public void deprecateConstant( | ||
final RubyContext context, | ||
final Node currentNode, | ||
final String name) { |
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.
Single line and no final
is the current style.
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.
I've fixed the style for this method defintion
|
||
@Specialization | ||
public DynamicObject deprecateConstant(VirtualFrame frame, DynamicObject module, Object[] args) { | ||
isFrozenNode.raiseIfFrozen(module); |
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.
There is CoreMethod.raiseIfFrozenSelf for this 😉
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.
I've changed to use raiseIfFrozenSelf
@@ -74,9 +74,17 @@ protected RubyConstant lookupConstant( | |||
if (!isVisible) { | |||
throw new RaiseException(coreExceptions().nameErrorPrivateConstant(module, name, this)); | |||
} | |||
if(constant != null && constant.isDeprecated()){ |
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.
Please try to fit in with the existing style as a general rule 😃
Maybe we should do this more automatically, like with mx eclipseformat, what do you think?
b06f580
to
c4a0217
Compare
public Assumption getUnmodifiedAssumption(DynamicObject module) { | ||
return Layouts.MODULE.getFields(module).getUnmodifiedAssumption(); | ||
} | ||
|
||
@TruffleBoundary |
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.
@eregon I've removed a few TruffleBoundary
s here to be able to pass the VirtualFrame
parameter. Does something in these methods need to be behind a boundary? Thanks.
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.
Yes, have a look at all calls from here.
doLookup
should be boundary, or otherwise lookupConstantAndObject
.
isVisible
as well.
There should be a profile before the throw since isVisible
is no longer compilation constant, and same goes for deprecated.
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.
In more details: boundaries are critical since missing ones could make compilation fail.
Profiles are less but they can significantly reduce the code generated, the compilation time and improve performance (this last one not so important here since we are already in a slow path).
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.
c4a0217
to
95c3463
Compare
@@ -45,21 +49,27 @@ public RubyConstant lookupConstant(VirtualFrame frame, Object module, String nam | |||
|
|||
@Specialization(assumptions = "getUnmodifiedAssumption(getModule())") | |||
protected RubyConstant lookupConstant( | |||
VirtualFrame frame, |
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.
On the line above since it's not @Cached
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.
I've fixed this in 6ab74ed
RubyConstant constant = doLookup(); | ||
if (!isVisible(constant)) { | ||
throw new RaiseException(coreExceptions().nameErrorPrivateConstant(getModule(), name, this)); | ||
} | ||
if (constant != null && constant.isDeprecated()) { | ||
warnDeprecatedConstant(frame, name); | ||
} |
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.
(same applies here regarding boundaries and profiles)
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.
I added boundaries and profiles here too.
|
||
@Child CallDispatchHeadNode warnMethod = CallDispatchHeadNode.createMethodCall(); | ||
|
||
public Object execute(VirtualFrame frame, Object... arguments) { |
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.
Since arguments seem to be always String for now, I would type them as such.
toString
is usually a bad practice.
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.
I've updated this to use String...
} | ||
return result; | ||
private String concatArgumentsToString(String... arguments) { | ||
return String.join("", arguments); |
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.
Nice Java 8 use!
if (!isVisible) { | ||
@Cached("createBinaryProfile()") ConditionProfile sameNameProfile, | ||
@Cached("createBinaryProfile()") ConditionProfile isVisibleProfile, | ||
@Cached("createBinaryProfile()") ConditionProfile isDeprecatedProfile) { |
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.
No need for profiles here since constant is compilation final!
throw new RaiseException(coreExceptions().nameErrorPrivateConstant(getModule(), name, this)); | ||
} | ||
if (isDeprecatedProfile.profile(constant != null && constant.isDeprecated())) { | ||
warnDeprecatedConstant(frame, name); | ||
} |
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.
same here and below regarding profiles
Getting there 😃 We just need to move the profiles to the uncached cases. |
Merged! |
I will add some questions in the diff.