Truffle API feedback

[christian.humer at gmail.com]

Hi Tim,

Thanks for your feedback. We really appreciate this.

Ad 1,4) I will leave these for Jaroslav as I think he needs to revamp 
the options system for multi tenancy anyway. TruffleOptions is indeed 
not a nice solution and I hope we can integrate it into a new options 
system.

Ad 2) In general I also like these kind of abstractions to avoid 
mistakes but I think in this case we should rather not box the 
probability value into an object. It would give us very little benefit 
and quite some complexity in the Truffle/Graal intrinisc implementation. 
Besides that, most guest languages are not using it directly. Instead 
they use ConditionProfile#createCountingProfile().

Ad 3) I agree. Will fix it for ReplaceObserver. Michael Van De Vanter 
can you fix this for SourceListener?

Ad 5,6) I will leave these for Michael Van De Vanter as well.

Please don't hesitate to shoot with more feedback at us :-)


- Christian Humer



------ Original Message ------
From: "Tim Boudreau" <niftiness at gmail.com>
To: graal-dev at openjdk.java.net
Sent: 19.06.2015 18:59:16
Subject: Truffle API feedback

>Hi, folks,
>
>I've been poking around randomly in the Truffle sources, and made a few
>notes on places where the API could be cleaner (which I think Jarda 
>Tulach
>is actively working on):
>
>1a.  CompilerOptions.setOption(String name, Object value);
>
>would be easier to use if it returned CompilerOptions instead of void, 
>so
>method chaining can be used.  In general, any time something like this
>returns void, you're throwing away an opportunity to make calling code 
>less
>verbose.
>
>1b. A cleaner and safer pattern would be:
>
>public abstract class CompilerOption<T> {
>    protected abstract void validate(T value) throws SomeException;
>//because you can
>}
>
>and
>
><T> CompilerOptions.setOption(CompilerOption<T>, T value);
>
>and let people define static fields with CompilerOption instances.
>
>That would be typesafe and would eliminate typos in string names.  And
>would probably allow language authors to factor their option processing
>more cleanly than a big switch statement.
>
>--------
>
>2. CompilerDirectives.injectBranchProbability(double probability, 
>boolean
>condition) and the constants such as LIKELY_PROBABILITY
>
>You're better off with a Probability class that hides the floating 
>point.
>There's no particular reason this sort of computation needs to be 
>floating
>point, and if it does, you're still better off with
>Probability.isMoreLikelyThan(otherProbability) or
>Probability.times(otherProbability).
>
>More flexibility later, you can put bounds on legal values (what would 
>a
>probability of Float.MIN_VALUE mean?  Make that impossible), and it's
>cleaner.
>
>--------
>
>3.  ReplaceObserver.nodeReplaced - observer/listener methods are 
>clearer if
>named on$EVENT, e.g. onNodeReplaced - so if implemented as a mix-in, it 
>is
>still obvious that this method responds to an event.
>
>Same for SourceListener, and probably other things named *Listener /
>*Observer.
>
>--------
>
>4.  Should TruffleOptions really have world-writable static fields?
>
>--------
>
>5.  BytesDecoder.decode() - should probably return CharSequence, not
>String.  With CharSequence you keep the option of avoiding a memory 
>copy,
>with an implementation of CharSequence over the raw array bytes.
>
>Once it's String, you can never go back.  Same for Source.getCode().
>
>--------
>
>6.  Source.setFileCaching() - this does not look like it should be API.
>Maybe some attribute of whatever is using the Graal API on 
>initialization
>(perhaps user code -> language -> truffle), but not here.
>
>--
>http://timboudreau.com