RFR 8131027: JShell API/tool: suggest imports for a class
Robert Field
robert.field at oracle.com
Fri Feb 12 22:19:03 UTC 2016
A sufficiently bad syntax error could mean no compilation unit. A null test
and I'm thumbs up!
-Robert
On February 12, 2016 1:47:39 PM Jan Lahoda <jan.lahoda at oracle.com> wrote:
> Hi Robert,
>
> Thanks for the comments. I've uploaded a new iteration here:
> http://cr.openjdk.java.net/~jlahoda/8131027/webrev.02/
>
> Delta webrev:
> http://cr.openjdk.java.net/~jlahoda/8131027/webrev.02/delta/webrev/index.html
>
> On 11.2.2016 08:43, Robert Field wrote:
>> 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.
>
> I've removed the loop.
>
>>
>> FixComputer[] -- FQN is used in user error: "\nNo candidate FQNs
>> found to import."
>
> Fixed.
>
>>
>>
>> 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)?)
>
> Fixed.
>
>>
>> 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"
>
> Fixed.
>
>>
>> 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.
>
> It turned out the "empty input" section was unnecessary -
> Completeness.EMPTY has isComplete == false, so the method should
> automatically abort for the empty input.
>
>
>> Seems you would want to add the semicolon when completeness ==
>> COMPLETE_WITH_SEMI.
>
> Done.
>
>> The TODO comment says "comment handling" -- this is what
>> MaskCommentsAndModifiers.java does -- just plug into that -- see
>> Eval.eval().
>
> The empty input section has been removed.
>
>> IMPORT and METHOD are rejected, but I'd think CLASS, ENUM,
>> ANNOTATION_TYPE, INTERFACE, and VARIABLE would be as well.
>
> Yes, thanks; done.
>
>>
>> ... 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
>
> I've added the help text into the review.
>
>>
> On 11.2.2016 20:50, Robert Field wrote:> Continuing (see below for first
> part) ...
>>
>> SourceCodeAnalysisImpl.java --
>>
>> Both listQualifiedNames() and analyzeType() -- at.firstCuTree() can
>> be null, are you safe if that happens?
>
> The code/wrap that is being passed to AnalyzeTask should ensure there is
> always a CompilationUnit, I think. Or do I miss some case where that
> wouldn't happen? (I've found a few bugs and added some more tests while
> looking at this.)
>
> Any comments are welcome.
>
> Thanks!
>
> Jan
>
>>
>>
>> TreeDissector.java --
>>
>> Looks good
>>
>>
>> Tests --
>>
>> Look good.
>>
>>
>> Thanks,
>> Robert
>>
>>
>>> On Feb 9, 2016, at 5:03 AM, Jan Lahoda <jan.lahoda at oracle.com
>>> <mailto: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/
>>>
>>> A delta webrev from the previous iteration:
>>> 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