matthiasblaesing commented on code in PR #7097:
URL: https://github.com/apache/netbeans/pull/7097#discussion_r1508190909
##########
ide/languages.hcl/src/org/netbeans/modules/languages/hcl/ast/HCLArithmeticOperation.java:
##########
@@ -61,51 +61,27 @@ public String toString() {
}
}
- public final Operator op;
-
- public HCLArithmeticOperation(Operator op) {
- this.op = op;
- }
-
- public final static class Binary extends HCLArithmeticOperation {
- public final HCLExpression left;
- public final HCLExpression right;
-
- public Binary(Operator op, HCLExpression left, HCLExpression right) {
- super(op);
- this.left = left;
- this.right = right;
- }
-
+ public record Binary(Operator op, HCLExpression left, HCLExpression right)
implements HCLArithmeticOperation {
@Override
public String asString() {
return left.toString() + op.toString() + right.toString();
- }
+ }
Review Comment:
Trailing space.
```suggestion
}
```
##########
ide/languages.hcl/test/unit/src/org/netbeans/modules/languages/hcl/HCLIndenterTest.java:
##########
@@ -118,7 +118,18 @@ public void testEmpty4() throws Exception {
@Test
public void testEmptyLine() throws Exception {
- performLineIndentationTest("locals {\n |\n}\n", "locals {\n\n}\n");
+ performLineIndentationTest(
+ """
+ locals {
+ __|
Review Comment:
Is the special casing necessary? The pipe symbol to mark the place of the
cursor I knew, the underscores look out of place. In fact I felt obligated to
read the text block JEP again to see whether I missed something.
##########
ide/languages.hcl/src/org/netbeans/modules/languages/hcl/ast/HCLContainer.java:
##########
@@ -18,59 +18,41 @@
*/
package org.netbeans.modules.languages.hcl.ast;
+import java.util.ArrayList;
import java.util.Collection;
-import java.util.Collections;
-import java.util.LinkedList;
import java.util.List;
/**
*
* @author Laszlo Kishalmi
*/
-public abstract class HCLContainer extends HCLAddressableElement {
- final List<HCLElement> elements = new LinkedList<>();
+public sealed interface HCLContainer extends HCLAddressableElement permits
HCLBlock, HCLDocument {
- final List<HCLBlock> blocks = new LinkedList<>();
- final List<HCLAttribute> attributes = new LinkedList<>();
-
- public HCLContainer(HCLContainer parent) {
- super(parent);
- }
-
- public void add(HCLBlock block) {
- elements.add(block);
- blocks.add(block);
- }
-
- public void add(HCLAttribute attr) {
- elements.add(attr);
- attributes.add(attr);
- }
-
- @Override
- public HCLContainer getContainer() {
- return (HCLContainer) parent;
- }
-
- public Collection<? extends HCLBlock> getBlocks() {
- return Collections.unmodifiableCollection(blocks);
+ public default Collection<? extends HCLBlock> blocks() {
+ var ret = new ArrayList<HCLBlock>();
+ for (HCLElement element : elements()) {
+ if (element instanceof HCLBlock b) {
+ ret.add(b);
+ }
+ }
+ return List.copyOf(ret);
Review Comment:
This might be an alternative. It is in line with the later usage of streams,
is 2 lines shorter and does not use var ...
```suggestion
return elements()
.stream()
.filter(e -> e instanceof HCLBlock)
.map(e -> (HCLBlock) e)
.toList();
```
The same pattern (with changed classes) can be applied for `attributes`.
##########
ide/languages.hcl/manifest.mf:
##########
@@ -3,5 +3,5 @@ OpenIDE-Module: org.netbeans.modules.languages.hcl
OpenIDE-Module-Layer: org/netbeans/modules/languages/hcl/layer.xml
OpenIDE-Module-Localizing-Bundle:
org/netbeans/modules/languages/hcl/Bundle.properties
OpenIDE-Module-Specification-Version: 1.4
-OpenIDE-Module-Java-Dependencies: Java > 11
+OpenIDE-Module-Java-Dependencies: Java > 17
Review Comment:
This was one of the things that raise questions. I'm not sure whether
bumping minimum version carries it weight. At least some of the changes could
have been implemented similar with JDK 11. But then I'm biased because my pet
(JNA) is still building for 8. And on the other hand the reduced code size is a
very good argument for the change.
##########
ide/languages.hcl/src/org/netbeans/modules/languages/hcl/HCLSemanticAnalyzer.java:
##########
@@ -115,9 +121,11 @@ public DefaultHighlighter(SourceRef refs) {
}
@Override
- protected boolean visitBlock(HCLBlock block) {
- if (block.getParent() instanceof HCLDocument) {
- List<HCLIdentifier> decl = block.getDeclaration();
+ protected void highlight(HCLTreeWalker.Step step) {
+
+ HCLElement<HCLElement> e = step.node();
+ if (e instanceof HCLBlock block && step.depth() == 1) {
Review Comment:
It might be clearer, if the ifs would be chained as `else if`s. Possible
double matches would then be prevented.
##########
ide/languages.hcl/src/org/netbeans/modules/languages/hcl/ast/HCLElement.java:
##########
@@ -18,81 +18,12 @@
*/
package org.netbeans.modules.languages.hcl.ast;
-import org.antlr.v4.runtime.Token;
+import java.util.List;
/**
*
* @author Laszlo Kishalmi
*/
-public abstract class HCLElement {
-
- public abstract void accept(Visitor v);
-
- public interface Visitor {
- /**
- * Visit the given element. Shall return {@code true} if the visit
- * shall be finished at this level in the element tree.
- *
- * @param e the element to visit.
- * @return {@code false} if the visit shall continue on the subtree of
- * the given element.
- */
- boolean visit(HCLElement e);
- }
-
- /**
- * Convenience Visitor implementation, where the HCLElements are split to
- * Block, Attribute, and Expression types.
- */
- public abstract static class BAEVisitor implements Visitor {
- @Override
- public boolean visit(HCLElement e) {
- if (e instanceof HCLBlock) {
- return visitBlock((HCLBlock)e);
- } else if (e instanceof HCLAttribute) {
- return visitAttribute((HCLAttribute) e);
- } else if (e instanceof HCLExpression) {
- return visitExpression((HCLExpression) e);
- }
- return false;
- }
-
- protected abstract boolean visitBlock(HCLBlock block);
-
- protected abstract boolean visitAttribute(HCLAttribute attr);
-
- protected abstract boolean visitExpression(HCLExpression expr);
- }
-
- public static class BAEVisitorAdapter extends BAEVisitor {
-
- @Override
- protected boolean visitBlock(HCLBlock block) {
- return false;
- }
-
- @Override
- protected boolean visitAttribute(HCLAttribute attr) {
- return false;
- }
-
- @Override
- protected boolean visitExpression(HCLExpression expr) {
- return false;
- }
-
- }
-
- public static final class CreateContext {
- public final HCLElement element;
- public final Token start;
- public final Token stop;
-
- public CreateContext(HCLElement element, Token start, Token stop) {
- this.element = element;
- this.start = start;
- this.stop = stop;
- }
-
- }
+public sealed interface HCLElement<T extends HCLElement> permits
HCLExpression, HCLIdentifier, HCLAddressableElement {
+ List<? extends T> elements();
Review Comment:
I think this was one of the places where I had this - "why?!" moment, but
reading it a second time, the `BAEVisitor` is indeed not nicer to use, than the
direct iteration in `DefaultHighlighter`. My judgement might change though when
the number of cases grows. A if-then-else chain can not grow indefinite if it
shall be kept readble.
##########
ide/languages.hcl/src/org/netbeans/modules/languages/hcl/ast/HCLTreeWalker.java:
##########
@@ -0,0 +1,93 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.netbeans.modules.languages.hcl.ast;
+
+import java.util.Iterator;
+import java.util.LinkedList;
+import java.util.function.Predicate;
+
+/**
+ *
+ * @author lkishalmi
+ */
+public final class HCLTreeWalker {
+ public record Step(HCLElement<HCLElement> parent, HCLElement<HCLElement>
node, int depth) {}
+
+ private HCLTreeWalker() {}
+
+ public static Iterator<Step> depthFirst(HCLElement<HCLElement> root) {
+ var process = new LinkedList<Step>();
+ process.push(new Step(null, root, 0));
+ return new Iterator<>() {
+
+ @Override
+ public boolean hasNext() {
+ return !process.isEmpty();
+ }
+
+ @Override
+ public Step next() {
+ Step current = process.pop();
+ current.node.elements().forEach((HCLElement e) ->
process.push(new Step(current.node, e, current.depth + 1)));
+ return current;
+ }
+ };
+ }
+
+ public static Iterator<Step> breadthFirst(HCLElement<HCLElement> root) {
+ var process = new LinkedList<Step>();
+ process.push(new Step(null, root, 0));
+ return new Iterator<>() {
+
+ @Override
+ public boolean hasNext() {
+ return !process.isEmpty();
+ }
+
+ @Override
+ public Step next() {
+ Step current = process.pop();
+ current.node.elements().forEach((HCLElement e) ->
process.add(new Step(current.node, e, current.depth + 1)));
+ return current;
+ }
+ };
+ }
Review Comment:
Both `Iterator` variants are currently unused. Intentional? Given that this
is not exported code, I suggest to consider cleaning this up.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
