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

8267329: Modernize Javadoc code to use instanceof with pattern matching #4105

Closed
Closed
Changes from 3 commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2012, 2020, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2012, 2021, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -113,15 +113,15 @@ public DocumentationTask getTask(

if (out == null)
context.put(Log.errKey, new PrintWriter(System.err, true));
else if (out instanceof PrintWriter)
context.put(Log.errKey, ((PrintWriter) out));
else if (out instanceof PrintWriter pout)
context.put(Log.errKey, pout);
else
context.put(Log.errKey, new PrintWriter(out, true));

if (fileManager == null) {
fileManager = getStandardFileManager(diagnosticListener, null, null);
if (fileManager instanceof BaseFileManager) {
((BaseFileManager) fileManager).autoClose = true;
if (fileManager instanceof BaseFileManager bfm) {
bfm.autoClose = true;
}
}
fileManager = ccw.wrap(fileManager);
@@ -470,9 +470,9 @@ private Content getClassLinks(HtmlLinkInfo.Kind context, Collection<?> list) {
isFirst = false;
}
// TODO: should we simply split this method up to avoid instanceof ?
if (type instanceof TypeElement) {
if (type instanceof TypeElement te) {
Content link = getLink(
new HtmlLinkInfo(configuration, context, (TypeElement)(type)));
new HtmlLinkInfo(configuration, context, te));
content.add(HtmlTree.CODE(link));
} else {
Content link = getLink(
@@ -407,10 +407,10 @@ public Content getContent(String key, Object o0, Object o1, Object o2) {

if (o == null) {
c.add("{" + m.group(1) + "}");
} else if (o instanceof String) {
c.add((String) o);
} else if (o instanceof Content) {
c.add((Content) o);
} else if (o instanceof String str) {
c.add(str);
} else if (o instanceof Content con) {
c.add(con);
}

Comment on lines 408 to 415

This comment has been minimized.

@jonathan-gibbons

jonathan-gibbons May 19, 2021
Contributor

I guess at some point this becomes "pattern switch" or whatever the feature is called ... but not yet

This comment has been minimized.

@pavelrappo

pavelrappo May 24, 2021
Member

The feature you are talking about is called "Pattern Matching for switch". The feature is described as a preview feature by JEP 406, which is currently proposed to target JDK 17.

start = m.end();
@@ -354,8 +354,7 @@ public Locale getLocale() {
@Override
public JavaFileObject getOverviewPath() {
String overviewpath = options.overviewPath();
if (overviewpath != null && getFileManager() instanceof StandardJavaFileManager) {
StandardJavaFileManager fm = (StandardJavaFileManager) getFileManager();
if (overviewpath != null && getFileManager() instanceof StandardJavaFileManager fm) {
return fm.getJavaFileObjects(overviewpath).iterator().next();
}
return null;
@@ -1123,8 +1123,8 @@ public Content seeTagToContent(Element element, DocTree see, TagletWriterImpl.Co
// documented, this must be an inherited link. Redirect it.
// The current class either overrides the referenced member or
// inherits it automatically.
if (this instanceof ClassWriterImpl) {
containing = ((ClassWriterImpl) this).getTypeElement();
if (this instanceof ClassWriterImpl cwrtr) {
containing = cwrtr.getTypeElement();
This conversation was marked as resolved by igraves

This comment has been minimized.

@jonathan-gibbons

jonathan-gibbons May 19, 2021
Contributor Outdated

suggest the name writer not cwrtr

} else if (!utils.isPublic(containing)) {
messages.warning(
ch.getDocTreePath(see), "doclet.see.class_or_package_not_accessible",
@@ -2129,11 +2129,11 @@ Script getMainBodyScript() {
List<DocPath> getLocalStylesheets(Element element) throws DocFileIOException {
List<DocPath> stylesheets = new ArrayList<>();
DocPath basePath = null;
if (element instanceof PackageElement) {
stylesheets.addAll(getModuleStylesheets((PackageElement)element));
basePath = docPaths.forPackage((PackageElement)element);
} else if (element instanceof ModuleElement) {
basePath = DocPaths.forModule((ModuleElement)element);
if (element instanceof PackageElement pkgElem) {
This conversation was marked as resolved by igraves

This comment has been minimized.

@jonathan-gibbons

jonathan-gibbons May 19, 2021
Contributor Outdated

Suggest pkg

stylesheets.addAll(getModuleStylesheets(pkgElem));
basePath = docPaths.forPackage(pkgElem);
} else if (element instanceof ModuleElement modElem) {
basePath = DocPaths.forModule(modElem);
Comment on lines +2135 to +2136

This comment has been minimized.

@jonathan-gibbons

jonathan-gibbons May 19, 2021
Contributor Outdated

Missed this last time; maybe "mdle" would be more in line with usage elsewhere

}
for (DocPath stylesheet : getStylesheets(element)) {
stylesheets.add(basePath.resolve(stylesheet.getPath()));
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2003, 2020, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2003, 2021, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -51,8 +51,7 @@ public LinkOutputImpl() {

@Override
public void append(Object o) {
output.append(o instanceof String ?
(String) o : ((LinkOutputImpl)o).toString());
output.append(o.toString());

This comment has been minimized.

@jonathan-gibbons

jonathan-gibbons May 19, 2021
Contributor Outdated

There may be a change in behavior here, for "invalid" arguments, which will now use .toString() instead of throwing CCE.

This comment has been minimized.

@igraves

igraves May 19, 2021
Author Member Outdated

My first reaction is in agreement, but upon further inspection it seems that this class is not used by anything in the module. Interestingly its documentation header warns not to rely on its behavior to remain the same.

This comment has been minimized.

@jonathan-gibbons

jonathan-gibbons May 19, 2021
Contributor Outdated

The documentation header is a standard langtools warnings to all those people that rely on access to javac and javadoc internals.

Class not used ... wow, correct. I'll file an RFE to investigate and remove it.

This comment has been minimized.

@pavelrappo

pavelrappo May 20, 2021
Member Outdated

There may be a change in behavior here, for "invalid" arguments, which will now use .toString() instead of throwing CCE.

Jon is right about the behavior change. That was a sloppy suggestion on my part. Sorry, Ian; you should revert that to your original version. Jon has created an RFR to remove LinkOutputImpl and LinkOutput: https://git.openjdk.java.net/jdk/pull/4121

This comment has been minimized.

@pavelrappo

pavelrappo May 20, 2021
Member Outdated

Correction. Instead of reverting to your original version, you should change it to this:

    @Override
    public void append(Object o) {
        output.append(o instanceof String s ? s : ((LinkOutputImpl) o).toString());
    }
}

@Override
@@ -175,9 +175,9 @@ public void convertModule(ModuleElement mdl, DocPath outputdir)
return;
}
for (Element elem : mdl.getEnclosedElements()) {
if (elem instanceof PackageElement && configuration.docEnv.isIncluded(elem)
if (elem instanceof PackageElement pkgElem && configuration.docEnv.isIncluded(elem)
This conversation was marked as resolved by igraves

This comment has been minimized.

@jonathan-gibbons

jonathan-gibbons May 19, 2021
Contributor Outdated

Use pkg

&& !(options.noDeprecated() && utils.isDeprecated(elem))) {
convertPackage((PackageElement) elem, outputdir);
convertPackage(pkgElem, outputdir);
}
}
}
@@ -155,8 +155,7 @@ private Content createLink(IndexItem i) {
if (element instanceof OverviewElement) {
return links.createLink(pathToRoot.resolve(i.getUrl()),
resources.getText("doclet.Overview"));
} else if (element instanceof DocletElement) {
DocletElement e = (DocletElement) element;
} else if (element instanceof DocletElement e) {
// Implementations of DocletElement do not override equals and
// hashCode; putting instances of DocletElement in a map is not
// incorrect, but might well be inefficient
@@ -320,12 +320,12 @@ public Content seeTagOutput(Element holder, List<? extends SeeTree> seeTags) {
links.add(htmlWriter.seeTagToContent(holder, dt, context.within(dt)));
}
if (utils.isVariableElement(holder) && ((VariableElement)holder).getConstantValue() != null &&
htmlWriter instanceof ClassWriterImpl) {
htmlWriter instanceof ClassWriterImpl clsHtml) {
This conversation was marked as resolved by igraves

This comment has been minimized.

@jonathan-gibbons

jonathan-gibbons May 19, 2021
Contributor Outdated

Use writer

//Automatically add link to constant values page for constant fields.
DocPath constantsPath =
htmlWriter.pathToRoot.resolve(DocPaths.CONSTANT_VALUES);
String whichConstant =
((ClassWriterImpl) htmlWriter).getTypeElement().getQualifiedName() + "." +
clsHtml.getTypeElement().getQualifiedName() + "." +
utils.getSimpleName(holder);
DocLink link = constantsPath.fragment(whichConstant);
links.add(htmlWriter.links.createLink(link,
@@ -506,8 +506,7 @@ public String visitVariable(VariableElement e, Void p) {

@Override
public String visitUnknown(Element e, Void p) {
if (e instanceof DocletElement) {
DocletElement de = (DocletElement) e;
if (e instanceof DocletElement de) {
return switch (de.getSubKind()) {
case OVERVIEW -> resources.getText("doclet.Overview");
case DOCFILE -> getHolderName(de);
@@ -52,8 +52,8 @@ public ContentBuilder(Content... contents) {
public ContentBuilder add(Content content) {
Objects.requireNonNull(content);
ensureMutableContents();
if (content instanceof ContentBuilder) {
contents.addAll(((ContentBuilder) content).contents);
if (content instanceof ContentBuilder conBldr) {
This conversation was marked as resolved by igraves

This comment has been minimized.

@jonathan-gibbons

jonathan-gibbons May 19, 2021
Contributor Outdated

use b or cb

contents.addAll(conBldr.contents);
} else
contents.add(content);
return this;
@@ -65,8 +65,8 @@ public ContentBuilder add(CharSequence text) {
ensureMutableContents();
Content c = contents.isEmpty() ? null : contents.get(contents.size() - 1);
TextBuilder tb;
if (c != null && c instanceof TextBuilder) {
tb = (TextBuilder) c;
if (c instanceof TextBuilder txtBldr) {
tb = txtBldr;
} else {
contents.add(tb = new TextBuilder());
}
Comment on lines +68 to 72

This comment has been minimized.

@jonathan-gibbons

jonathan-gibbons May 19, 2021
Contributor Outdated

Restructure this

contents.add((c instanceof TextBuilder tb) ? tb : new TextBuilder());

or something similar

This comment has been minimized.

@igraves

igraves May 19, 2021
Author Member Outdated

Unclear if there's a more succinct way to refactor this like so. If c is not an instance of TextBuilder then we need to initialize a new TextBuilder and add it to contents. This is not the case if c is already of the desired instance.

This comment has been minimized.

@jonathan-gibbons

jonathan-gibbons May 19, 2021
Contributor Outdated

Then just a shorter name than txtBldr please. Lvng t vwls s nt - jvdc cnvntn.

This comment has been minimized.

@jonathan-gibbons

jonathan-gibbons May 19, 2021
Contributor Outdated

I guess we have competing demands for short names!

@@ -174,8 +174,8 @@ public HtmlTree addStyle(String style) {
*/
@Override
public HtmlTree add(Content content) {
if (content instanceof ContentBuilder) {
((ContentBuilder) content).contents.forEach(this::add);
if (content instanceof ContentBuilder conBldr) {
This conversation was marked as resolved by igraves

This comment has been minimized.

@jonathan-gibbons

jonathan-gibbons May 19, 2021
Contributor Outdated

Use cb

conBldr.contents.forEach(this::add);
}
else if (content == HtmlTree.EMPTY || content.isValid()) {
// quietly avoid adding empty or invalid nodes (except EMPTY)
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2015, 2020, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2015, 2021, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -369,7 +369,7 @@ private boolean hasParamForComponent(TypeElement elem, Name component) {
}

for (DocTree t : elemComment.getBlockTags()) {
if (t instanceof ParamTree && ((ParamTree) t).getName().getName() == component) {
if (t instanceof ParamTree pt && pt.getName().getName() == component) {
return true;
}
}
@@ -420,8 +420,7 @@ public DocCommentInfo getHtmlCommentInfo(Element e) {
PackageElement pe = null;
switch (e.getKind()) {
case OTHER:
if (e instanceof DocletElement) {
DocletElement de = (DocletElement)e;
if (e instanceof DocletElement de) {
fo = de.getFileObject();
pe = de.getPackageElement();
}
@@ -316,8 +316,8 @@ private void processProperty(Element member,
fullBody.addAll(cmtutils.makeFirstSentenceTree(text));
}
List<? extends DocTree> propertyTags = utils.getBlockTags(property,
t -> (t instanceof UnknownBlockTagTree)
&& ((UnknownBlockTagTree) t).getTagName().equals("propertyDescription"));
t -> (t instanceof UnknownBlockTagTree ukbt)
This conversation was marked as resolved by igraves

This comment has been minimized.

@jonathan-gibbons

jonathan-gibbons May 19, 2021
Contributor Outdated

Use tree

&& (ukbt.getTagName().equals("propertyDescription")));
if (propertyTags.isEmpty()) {
List<? extends DocTree> comment = utils.getFullBody(property);
blockTags.addAll(cmtutils.makePropertyDescriptionTree(comment));
@@ -331,8 +331,8 @@ private void processProperty(Element member,
blockTags.addAll(tags);

List<? extends DocTree> bTags = utils.getBlockTags(property,
t -> (t instanceof UnknownBlockTagTree)
&& ((UnknownBlockTagTree) t).getTagName().equals("defaultValue"));
t -> (t instanceof UnknownBlockTagTree ukbt)
This conversation was marked as resolved by igraves

This comment has been minimized.

@jonathan-gibbons

jonathan-gibbons May 19, 2021
Contributor Outdated

Use tree

&& (ukbt.getTagName().equals("defaultValue")));
blockTags.addAll(bTags);

//add @see tags
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2001, 2020, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2001, 2021, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -223,8 +223,7 @@ public TagletManager(BaseConfiguration configuration) {
* @throws IOException if an error occurs while setting the location
*/
public void initTagletPath(JavaFileManager fileManager) throws IOException {
if (fileManager instanceof StandardJavaFileManager) {
StandardJavaFileManager sfm = (StandardJavaFileManager)fileManager;
if (fileManager instanceof StandardJavaFileManager sfm) {
if (tagletPath != null) {
List<File> paths = new ArrayList<>();
for (String pathname : tagletPath.split(File.pathSeparator)) {
@@ -553,8 +552,7 @@ private void printTagMisuseWarn(CommentHelper ch, Taglet taglet, DocTree tag, St
case PACKAGE:
return blockTagletsByLocation.get(Location.PACKAGE);
case OTHER:
if (e instanceof DocletElement) {
DocletElement de = (DocletElement) e;
if (e instanceof DocletElement de) {
switch (de.getSubKind()) {
case DOCFILE:
return blockTagletsByLocation.get(Location.PACKAGE);
@@ -753,7 +751,7 @@ private void showTaglets(PrintStream out) {
+ format(t.inMethod(), "method") + " "
+ format(t.inField(), "field") + " "
+ format(t.isInlineTag(), "inline")+ " "
+ format((t instanceof SimpleTaglet) && !((SimpleTaglet) t).enabled, "disabled"));
+ format((t instanceof SimpleTaglet st) && !st.enabled, "disabled"));
});
}

@@ -1,5 +1,5 @@
/*
* Copyright (c) 2003, 2020, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2003, 2021, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -263,8 +263,7 @@ public Content getBlockTagOutput(TagletManager tagletManager,
continue;
}

if (element.getKind() == ElementKind.MODULE && taglet instanceof BaseTaglet) {
BaseTaglet t = (BaseTaglet) taglet;
if (element.getKind() == ElementKind.MODULE && taglet instanceof BaseTaglet t) {
switch (t.getTagKind()) {
// @uses and @provides are handled separately, so skip here.
// See ModuleWriterImpl.computeModulesData
@@ -280,7 +279,7 @@ public Content getBlockTagOutput(TagletManager tagletManager,
continue;
}

if (taglet instanceof SimpleTaglet && !((SimpleTaglet) taglet).enabled) {
if (taglet instanceof SimpleTaglet st && !st.enabled) {
// taglet has been disabled
continue;
}
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2001, 2020, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2001, 2021, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -74,8 +74,8 @@ public void inherit(DocFinder.Input input, DocFinder.Output output) {
Element exception;
CommentHelper ch = utils.getCommentHelper(input.element);
if (input.tagId == null) {
exception = input.docTreeInfo.docTree instanceof ThrowsTree
? ch.getException((ThrowsTree) input.docTreeInfo.docTree) : null;
exception = input.docTreeInfo.docTree instanceof ThrowsTree tt
? ch.getException(tt) : null;
input.tagId = exception == null
? ch.getExceptionName(input.docTreeInfo.docTree).getSignature()
: utils.getFullyQualifiedName(exception);
@@ -1,5 +1,5 @@
/*
* Copyright (c) 1998, 2020, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1998, 2021, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -63,7 +63,7 @@ protected DocPath(String p) {

@Override
public boolean equals(Object other) {
return (other instanceof DocPath) && path.equals(((DocPath)other).path);
return (other instanceof DocPath dp) && path.equals(dp.path);
}

@Override
@@ -1,5 +1,5 @@
/*
* Copyright (c) 1998, 2020, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1998, 2021, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -396,14 +396,13 @@ private boolean link(String url, String elemlisturl, Reporter reporter, boolean
*/
private Item findElementItem(Element element) {
Item item = null;
if (element instanceof ModuleElement) {
item = moduleItems.get(utils.getModuleName((ModuleElement)element));
if (element instanceof ModuleElement modElem) {
This conversation was marked as resolved by igraves

This comment has been minimized.

@jonathan-gibbons

jonathan-gibbons May 19, 2021
Contributor Outdated

Use mdle or me

item = moduleItems.get(utils.getModuleName(modElem));
}
else if (element instanceof PackageElement) {
PackageElement packageElement = (PackageElement)element;
ModuleElement moduleElement = utils.containingModule(packageElement);
else if (element instanceof PackageElement pkgElem) {
This conversation was marked as resolved by igraves

This comment has been minimized.

@jonathan-gibbons

jonathan-gibbons May 19, 2021
Contributor Outdated

Use pkg or pe

ModuleElement moduleElement = utils.containingModule(pkgElem);
Map<String, Item> pkgMap = packageItems.get(utils.getModuleName(moduleElement));
item = (pkgMap != null) ? pkgMap.get(utils.getPackageName(packageElement)) : null;
item = (pkgMap != null) ? pkgMap.get(utils.getPackageName(pkgElem)) : null;
}
return item;
}
@@ -624,8 +623,7 @@ private InputStream open(URL url) throws IOException {
in = conn.getInputStream();
redir = false;

if (conn instanceof HttpURLConnection) {
HttpURLConnection http = (HttpURLConnection)conn;
if (conn instanceof HttpURLConnection http) {
int stat = http.getResponseCode();
// See:
// https://developer.mozilla.org/en-US/docs/Web/HTTP/Status