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

Added method for graphical representation of Truffle Methods #110

Merged
merged 3 commits into from
Mar 7, 2017

Conversation

VAISHALI-DHANOA
Copy link
Contributor

@VAISHALI-DHANOA VAISHALI-DHANOA commented Mar 1, 2017

This change adds support to show methods directly after parsing in IGV.

This can be used with the -im/--igv-parsed-methods options on the som script.

Copy link
Owner

@smarr smarr left a comment

Choose a reason for hiding this comment

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

Hey, thanks a lot for the PR. The key parts look good, however, there seem to be a couple of unintended changes, which might be the result of having worked with an earlier version. Could you please verify what happened there?

src/som/VM.java Outdated
@@ -31,15 +35,14 @@
import som.interpreter.actors.SFarReference;
import som.interpreter.actors.SPromise;
import som.interpreter.actors.SPromise.SResolver;
import som.primitives.processes.ChannelPrimitives;
Copy link
Owner

Choose a reason for hiding this comment

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

This removal looks suspicious. Is this intended?

src/som/VM.java Outdated
@@ -86,7 +89,7 @@ public static void setEngine(final PolyglotEngine e) {

private int lastExitCode = 0;
private volatile boolean shouldExit = false;
private final VmOptions options;
private final VMOptions options;
Copy link
Owner

Choose a reason for hiding this comment

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

This change looks like it might be the result of having worked with an earlier version. I recently renamed VmOptions.

src/som/VM.java Outdated
public VM(final String[] args, final boolean avoidExitForTesting) throws IOException {
vm = this;

this.avoidExitForTesting = avoidExitForTesting;
options = new VmOptions(args);
objectSystem = new ObjectSystem(new SourcecodeCompiler(), structuralProbe);
objectSystem.loadKernelAndPlatform(options.platformFile, options.kernelFile);
Copy link
Owner

Choose a reason for hiding this comment

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

Hm, why is the loading of kernel and platform removed? Is this intentional?

src/som/VM.java Outdated
} catch (IOException e) {
VM.errorPrint("Failed to setup coverage tracking: " + e.getMessage());
}
cov.setRepoToken(vmOptions.coverallsRepoToken);
Copy link
Owner

Choose a reason for hiding this comment

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

Likely another set of unintended changes.

src/som/VM.java Outdated
@@ -377,8 +405,6 @@ public static void resetClassReferences(final boolean callFromUnitTest) {
ThreadingModule.ConditionClass = null;
ThreadingModule.ConditionClassId = null;

ChannelPrimitives.resetClassReferences();
Copy link
Owner

Choose a reason for hiding this comment

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

Likely another unintended change.

@smarr
Copy link
Owner

smarr commented Mar 6, 2017

Thanks, looks good will test it tomorrow!

@smarr smarr added this to the v0.2.0 milestone Mar 6, 2017
@smarr smarr added the enhancement Improves the implementation with something noteworthy label Mar 7, 2017
@smarr smarr merged commit a674211 into smarr:master Mar 7, 2017
@smarr
Copy link
Owner

smarr commented Mar 7, 2017

@VAISHALI-DHANOA thanks again for the pull request. I simplified it a bit, also to avoid showing the same methods over and over again in IGV. And, I added a switch to the launcher script. So, if you got IGV running, and you execute som with the -im option, you get the methods shown.

@VAISHALI-DHANOA VAISHALI-DHANOA deleted the igv-method branch March 7, 2017 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improves the implementation with something noteworthy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants