RFR 8131027: JShell API/tool: suggest imports for a class
Robert Field
robert.field at oracle.com
Thu Feb 11 07:43:21 UTC 2016
HI Jan,
Review of version 01 ---
ConsoleIOContext.java --
fixes() -- on unexpected character it just cycles, waiting for another, for a user stuck where not wanted that could be confusing. If a user is here by mistake and typing ahead then ignoring unexpected characters increases the chance that a random character will have an unexpected effect. Some options (depending on the perceived likelihood of legitimately typing a wrong char: (1) no loop, if an unexpected char beep and exit (2) loop for a small number of times, three?, beeping on bad input. (3) print some help "Type a number" on bad input.
FixComputer[] -- FQN is used in user error: "\nNo candidate FQNs found to import."
Eval.java --
OK
JShell.java --
OK
SourceCodeAnalysis.java --
analyzeType() & listQualifiedNames() -- Nice javadoc.
QualifiedNames --
The constructor is only use by us, exposing in the API is messy and potentially confusing. Luckily the solution is just to remove "public" off the constructor, since package-private is all we need. (while you are at it, can you tuck CompletionInfo constructor in too (impl used to be in a separate package)?)
isResolvable() -- the methods above refer to "the simple name in the original code". This calls it "the given identifier", I assume this is the same thing? I'd stick with "the simple name in the original code"
SourceCodeAnalysisImpl.java --
analyzeType() --
It aborts with null when not complete, which seems fine. Why does it proceed (and add a semicolon) when the input in empty -- I'd think it should also abort.
Seems you would want to add the semicolon when completeness == COMPLETE_WITH_SEMI.
The TODO comment says "comment handling" -- this is what MaskCommentsAndModifiers.java does -- just plug into that -- see Eval.eval().
IMPORT and METHOD are rejected, but I'd think CLASS, ENUM, ANNOTATION_TYPE, INTERFACE, and VARIABLE would be as well.
... to be continued ...
-------
On mi Fedora (KDE-spin) machine I could get neither Alt-f1 or Alt-Enter to work.
Send me a sketch of what user docs would say and I'll incorporate it in /help
Thanks,
Robert
> On Feb 9, 2016, at 5:03 AM, Jan Lahoda <jan.lahoda at oracle.com> wrote:
>
> Hi Robert,
>
> Thanks for the comments - I've uploaded an updated webrev here:
> http://cr.openjdk.java.net/~jlahoda/8131027/webrev.01/ <http://cr.openjdk.java.net/~jlahoda/8131027/webrev.01/>
>
> A delta webrev from the previous iteration:
> http://cr.openjdk.java.net/~jlahoda/8131027/webrev.01/delta/webrev/index.html <http://cr.openjdk.java.net/~jlahoda/8131027/webrev.01/delta/webrev/index.html>
>
> Some more comments inline.
>
> On 28.1.2016 22:38, Robert Field wrote:
>> This will be a big help -- thanks for coding this.
>>
>> Below is a pre-review.
>>
>> JShell:
>>
>> Fine.
>>
>>
>> Eval:
>>
>> Looks good.
>>
>>
>> SourceCodeAnalysis:
>>
>> This is public API, so names and comments really matter...
>
> I tried to improve the names and javadoc.
>
>>
>> analyzeType() -- There is a description of where the expression
>> begins, but not of where it ends.
>>
>> getDeclaredSymbols() -- Please document what "possible" in
>> "possible FQNs" means.
>>
>> IndexResult --
>>
>> This class needs a name that is more descriptive of what it is,
>> and the name should align with whatever name and comment are finally
>> given to getDeclaredSymbols(). NameLookup? ResolvedName? ...
>>
>> The class comment should describe what this class is -- and it
>> more than a list of FQNs, as it also deals with the simple name,
>> up-to-dateness, resolvability.
>> getFqns() -- As above, and probably here rather than there, --
>> what does possible mean?
>>
>> getSimpleNameLength() -- Says "unresolvable", but there is a
>> test (below) for it being resolvable -- probably just remove that word.
>> "right left to", were you meaning "right of"? "for which the
>> candidates could be computed" if they can't be computed, how does that
>> effect this method? Wouldn't return still return the length? Can that
>> phrase just be removed? Is the a reason that the length (rather than
>> the name) is returned?
>>
>> isUpToDate() -- I have no idea what this means.
>>
>> isResolvable() -- Resolvable in the context of what?
>>
>> Throughout class, I would avoid "FQN". See, for example, the use
>> of "fully qualified name" in java/lang. I would spell it out in
>> comments. The java/lang uses "name" as the base for identifiers, but
>> this won't work for getFqns() since it needs to be differentiated from
>> simple names. Perhaps getQualifiedNames().
>>
>> Nit: Throughout the JShell code and throughout the JDK codebase the
>> convention is that comments start on the line after the "/**" (not on
>> the same line).
>>
>> SourceCodeAnalysisImpl:
>>
>> getDeclaredSymbols() -- My reading of the spec (above) is that the
>> cursor is at the beginning of the identifier to match, this is at the
>> end. Spec needs clarification or ...
>
> The cursor is expected to be immediately after the identifier, I tried to clarify that in the javadoc.
>
>>
>> If there is an @author tag on this file, you should certainly be
>> first.
>>
>> currentIndexes and two other fields are declared in the middle of
>> the class. While old conventions forbade this:
>> https://web.archive.org/web/20130627193836/http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-141855.html
>>
>> others don't when there is good reason. But I mention this because
>> there are many forward references to all three of these -- so their
>> implied locality is misleading.
>>
>> I would need some code context (comments) to be able to properly
>> review.
>
> I've added comments. Please let me know where I need to add more.
>
>>
>> ConsoleIOContext:
>>
>> Need more context (code comments) for me to be able to review.
>
> I've added comments. Please let me know where I need to add more.
>
>>
>> TreeDissector:
>>
>> See TypePrinter.
>>
>> TypePrinter:
>>
>> My (unspoken) design for this was to be as stand-alone (untied to
>> the JShell architecture) as possible -- esp. since it is subclassed off
>> javac, which is why I had that stuff in higher levels. It could be
>> moved up just one level (to TreeDissector)
>
> Ok, I've moved the method to TreeDissector - TypePrinter would still seem to be a nice place for it, to me, though.
>
>>
>> ComputeFQNsTest:
>>
>> Need tests with multiple answers. If the other fields of
>> IndexResult are interesting enough to be present they should be tested.
>>
>> InferTypeTest:
>>
>> Looks pretty good. Maybe some other express forms: boolean
>> operators, ....
>
> Thanks - there turned out to be a bug with binary operators, should be fixed now.
>
> Any comments are welcome!
>
> Thanks,
> Jan
>
>>
>> Thanks,
>> Robert
>>
>>
>> On 01/22/16 03:40, Jan Lahoda wrote:
>>> Hello,
>>>
>>> I'd like to ask for a review of a patch that adds two new features:
>>> a) add import: when there is an unresolvable identifier, press
>>> "<base-shortcut> i" (*), and a list of known FQNs should be printed,
>>> with an ability to easily import any of them.
>>>
>>> b) create variable: when there is an expression, press
>>> "<base-shortcut> v", and a variable stub will be created, with the
>>> expression as the initializer.
>>>
>>> The "<base-shortcut>" is either Alt-Enter or Alt-F1, depending on the
>>> platform. For Windows,
>>>
>>> The patch is here:
>>> http://cr.openjdk.java.net/~jlahoda/8131027/webrev.00/
>>>
>>> For Windows, an additional patch to jdk.internal.le is needed (I'll
>>> send a separate review to core-libs for that):
>>> http://cr.openjdk.java.net/~jlahoda/8147984/webrev.00/
>>>
>>> Any comments are welcome!
>>>
>>> Thanks,
>>> Jan
More information about the kulla-dev
mailing list