mbien commented on code in PR #7097: URL: https://github.com/apache/netbeans/pull/7097#discussion_r1522417782
########## ide/languages.hcl/src/org/netbeans/modules/languages/hcl/ast/HCLTreeWalker.java: ########## @@ -0,0 +1,54 @@ +/* + * 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.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 void depthFirst(HCLElement root, Predicate<Step> t) { + LinkedList<Step> process = new LinkedList<>(); + process.push(new Step(null, root, 0)); + while (!process.isEmpty()) { + Step current = process.pop(); + if (t.test(current)) { + current.node.elements().forEach((HCLElement e) -> process.push(new Step(current.node, e, current.depth + 1))); Review Comment: I think a normal for-each loop over the collection would be easier to debug and read here. ########## ide/languages.hcl/src/org/netbeans/modules/languages/hcl/ast/HCLTreeWalker.java: ########## @@ -0,0 +1,54 @@ +/* + * 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.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 void depthFirst(HCLElement root, Predicate<Step> t) { + LinkedList<Step> process = new LinkedList<>(); + process.push(new Step(null, root, 0)); + while (!process.isEmpty()) { + Step current = process.pop(); + if (t.test(current)) { + current.node.elements().forEach((HCLElement e) -> process.push(new Step(current.node, e, current.depth + 1))); + } + } + } + + public static void breadthFirst(HCLElement root, Predicate<Step> t) { + LinkedList<Step> process = new LinkedList<>(); + process.add(new Step(null, root, 0)); + while (!process.isEmpty()) { + var current = process.pop(); + if (t.test(current)) { + current.node.elements().forEach((HCLElement e) -> process.add(new Step(current.node, e, current.depth + 1))); Review Comment: here too ########## ide/languages.hcl/src/org/netbeans/modules/languages/hcl/ast/HCLBlock.java: ########## @@ -18,35 +18,22 @@ */ package org.netbeans.modules.languages.hcl.ast; -import java.util.Collections; import java.util.List; import java.util.stream.Collectors; /** * * @author Laszlo Kishalmi */ -public class HCLBlock extends HCLContainer { +public record HCLBlock(List<HCLIdentifier> declaration, List<HCLElement> elements) implements HCLContainer { - private List<HCLIdentifier> declaration; - private String id; - - public HCLBlock(HCLContainer parent) { - super(parent); - } - - void setDeclaration(List<HCLIdentifier> declaration) { - this.declaration = Collections.unmodifiableList(declaration); - id = declaration.stream().map(d -> d.id()).collect(Collectors.joining(".")); + public HCLBlock { + declaration = List.copyOf(declaration); + elements = List.copyOf(elements); } - @Override public String id() { - return id; - } - - public List<HCLIdentifier> getDeclaration() { - return declaration; + return declaration.stream().map(d -> d.id()).collect(Collectors.joining(".")); } Review Comment: I am not sure about this. You went through a lot to remove any state from the whole hierarchy. Interfaces and default methods on top and records on the bottom. This creates the problem that constants have to be recomputed all the time. I checked usages and there are loops which call `id()` multiple times in their body. This method here itself calls `id()` on something else to build its id. This would be just a final String initialized in the constructor. I am not sure if it is worth to use records here when classes with public final fields could achieve something similar without having to recompute everything all the time. Other cases are `attributes()` or `blocks()`. Default methods are often used to create a basic impl which is overwritten further down to provide an optimized impl. But with records you are stuck there too since they can't have any hidden fields. you could do this: ```java public record HCLBlock(List<HCLIdentifier> declaration, List<HCLElement> elements, String id) implements HCLContainer { public HCLBlock(List<HCLIdentifier> declaration, List<HCLElement> elements) { this(declaration, elements, declaration.stream().map(d -> d.id()).collect(Collectors.joining("."))); } } ``` but this a hack since it is not possible to hide the canonical 3 component constructor - someone can call it and break the id. Maybe the solution is to not make everything a record, just on a case by case basis. ########## ide/languages.hcl/src/org/netbeans/modules/languages/hcl/HCLSemanticAnalyzer.java: ########## @@ -87,25 +85,33 @@ public void cancel() { cancelled = true; } - protected abstract class Highlighter extends HCLElement.BAEVisitor { - protected final Map<OffsetRange, Set<ColoringAttributes>> work = new HashMap<>(); + protected abstract class Highlighter { + protected final Map<OffsetRange, Set<ColoringAttributes>> work = new IdentityHashMap<>(); Review Comment: is this due to the way how `OffsetRange` "calculates" its hash code? ########## ide/languages.hcl/src/org/netbeans/modules/languages/hcl/ast/HCLConditionalOperation.java: ########## @@ -25,25 +25,15 @@ * * @author lkishalmi */ -public final class HCLConditionalOperation extends HCLExpression { - - final HCLExpression condition; - final HCLExpression trueValue; - final HCLExpression falseValue; - - public HCLConditionalOperation(HCLExpression condition, HCLExpression trueValue, HCLExpression falseValue) { - this.condition = condition; - this.trueValue = trueValue; - this.falseValue = falseValue; - } +public record HCLConditionalOperation(HCLExpression condition, HCLExpression trueValue, HCLExpression falseValue) implements HCLExpression { @Override public String asString() { return condition.toString() + "?" + trueValue.toString() + ":" + falseValue.toString(); } @Override - public List<? extends HCLExpression> getChildren() { + public List<? extends HCLExpression> elements() { return Arrays.asList(condition, trueValue, falseValue); Review Comment: `List.of`? Another match would be in `HCLLanguage` L236. -- 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
