RFR 8131027: JShell API/tool: suggest imports for a class

[Robert Field]

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...

     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 ...

     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.

ConsoleIOContext:

     Need more context (code comments) for me to be able to review.

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)

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,
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