Copilot commented on code in PR #747:
URL: https://github.com/apache/commons-compress/pull/747#discussion_r2515586820
##########
src/main/java/org/apache/commons/compress/harmony/unpack200/NewAttributeBands.java:
##########
@@ -553,56 +628,58 @@ public void readBands(final InputStream in, final int
count) throws IOException,
*/
public class UnionCase extends LayoutElement {
- private List<LayoutElement> body;
+ private final List<Range<Integer>> tagRanges;
+ private final List<LayoutElement> body;
- private final List<Integer> tags;
+ public UnionCase(final List<Integer> tags) throws IOException {
+ this(tags, Collections.emptyList());
+ }
- public UnionCase(final List<Integer> tags) {
- this.tags = tags;
+ public UnionCase(final List<Integer> tags, final List<LayoutElement>
body) throws IOException {
Review Comment:
The constructor `UnionCase(final List<Integer> tags, final
List<LayoutElement> body) throws IOException` declares throwing `IOException`,
but it only delegates to another constructor that doesn't throw `IOException`.
The `IOException` declaration appears unnecessary and should be removed from
the signature.
```suggestion
public UnionCase(final List<Integer> tags, final List<LayoutElement>
body) {
```
##########
src/main/java/org/apache/commons/compress/harmony/internal/AttributeLayoutParser.java:
##########
@@ -0,0 +1,362 @@
+/*
+ * 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
+ *
+ * https://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.apache.commons.compress.harmony.internal;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+
+import org.apache.commons.compress.harmony.pack200.Pack200Exception;
+import org.apache.commons.lang3.Range;
+
+/**
+ * Encapsulates parsing of attribute layout definitions.
+ *
+ * <p>Previously the parsing logic was duplicated in {@link
org.apache.commons.compress.harmony.pack200.NewAttributeBands} and
+ * {@link org.apache.commons.compress.harmony.unpack200.NewAttributeBands}.</p>
+ *
+ * @param <LAYOUT_ELEMENT> the type corresponding to the {@code
layout_element} production
+ * @param <ATTRIBUTE_LAYOUT_ELEMENT> the type corresponding to the elements of
the {@code attribute_layout} production: either {@code layout_element} or
+ * {@code callable}
+ */
+public final class AttributeLayoutParser<ATTRIBUTE_LAYOUT_ELEMENT,
LAYOUT_ELEMENT extends ATTRIBUTE_LAYOUT_ELEMENT> {
+
+ /**
+ * Factory interface for creating attribute layout elements.
+ */
+ public interface Factory<ATTRIBUTE_LAYOUT_ELEMENT, LAYOUT_ELEMENT extends
ATTRIBUTE_LAYOUT_ELEMENT> {
+ LAYOUT_ELEMENT createCall(int callableIndex);
+
+ ATTRIBUTE_LAYOUT_ELEMENT createCallable(String body) throws
Pack200Exception;
+
+ LAYOUT_ELEMENT createIntegral(String tag);
+
+ LAYOUT_ELEMENT createReference(String tag);
+
+ LAYOUT_ELEMENT createReplication(String unsignedInt, String body)
throws Pack200Exception;
+
+ LAYOUT_ELEMENT createUnion(String anyInt, List<UnionCaseData> cases,
String body) throws Pack200Exception;
+ }
+
+ /**
+ * Data class representing a union case in an attribute layout definition.
+ */
+ public static final class UnionCaseData {
+ public final String body;
+ public final List<Range<Integer>> tagRanges;
+
+ private UnionCaseData(final List<Range<Integer>> tagRanges, final
String body) {
+ this.tagRanges = Collections.unmodifiableList(tagRanges);
+ this.body = body;
+ }
+ }
+
+ private final CharSequence definition;
+ private final Factory<ATTRIBUTE_LAYOUT_ELEMENT, LAYOUT_ELEMENT> factory;
+ private int p;
+
+ public AttributeLayoutParser(final CharSequence definition, final
Factory<ATTRIBUTE_LAYOUT_ELEMENT, LAYOUT_ELEMENT> factory) {
+ this.definition = definition;
+ this.factory = factory;
+ }
+
+ private void ensureNotEof() throws Pack200Exception {
+ if (eof()) {
+ throw new Pack200Exception("Incomplete attribute layout
definition: " + definition);
+ }
+ }
+
+ private boolean eof() {
+ return p >= definition.length();
+ }
+
+ private char expect(char... expected) throws Pack200Exception {
+ final char c = next();
+ for (char e : expected) {
+ if (c == e) {
+ return c;
+ }
+ }
+ throw new Pack200Exception("Invalid attribute layout definition:
expected one of " + new String(expected) + ", found '" + c + "'");
+ }
+
+ private char next() throws Pack200Exception {
+ ensureNotEof();
+ return definition.charAt(p++);
+ }
+
+ private char peek() throws Pack200Exception {
+ ensureNotEof();
+ return definition.charAt(p);
+ }
+
+ private String readAnyInt() throws Pack200Exception {
+ final char first = next();
+ return AttributeLayoutUtils.checkAnyIntTag(first == 'S' ? "S" + next()
: String.valueOf(first));
+ }
+
+ /**
+ * Reads the next {@code attribute_layout_element} from the stream.
+ *
+ * <pre>
+ * attribute_layout_element:
+ * ( callable | layout_element )
+ * callable:
+ * '[' (body)? ']'
+ * </pre>
+ *
+ * @return next AttributeLayoutElement from the stream or {@code null} if
end of stream is reached.
+ * @throws Pack200Exception If the layout definition is invalid.
+ */
+ public ATTRIBUTE_LAYOUT_ELEMENT readAttributeLayoutElement() throws
Pack200Exception {
+ if (eof()) {
+ return null;
+ }
+ final char first = peek();
+ if (first == '[') {
+ try {
+ final String body = readBody();
+ if (body.isEmpty()) {
+ throw new Pack200Exception("Corrupted Pack200 archive:
Callable body cannot be empty.");
+ }
+ return factory.createCallable(body);
+ } catch (final Exception ex) {
+ throw new Pack200Exception(String.format("Corrupted Pack200
archive: Invalid layout '%s' at position %d.", definition, p), ex);
+ }
+ }
+ return readLayoutElement();
+ }
+
+ private String readBody() throws Pack200Exception {
+ expect('[');
+ int depth = 1;
+ final StringBuilder body = new StringBuilder();
+ char c;
+ while (true) {
+ c = next();
+ if (c == '[') {
+ depth++;
+ } else if (c == ']') {
+ depth--;
+ if (depth == 0) {
+ break;
+ }
+ }
+ body.append(c);
+ }
+ return body.toString();
+ }
+
+ private String readIntegralTag() throws Pack200Exception {
+ char c = next();
+ final char[] buf = new char[3];
+ int len = 0;
+ buf[len++] = c;
+ if (c == 'F' || c == 'O' || c == 'P' || c == 'S') {
+ c = next();
+ buf[len++] = c;
+ if (c == 'O' || c == 'S') {
+ c = next();
+ buf[len++] = c;
+ }
+ }
+ return AttributeLayoutUtils.checkIntegralTag(new String(buf, 0, len));
+ }
+
+ /**
+ * Reads the next {@code layout_element} from the stream.
+ *
+ * <pre>
+ * layout_element:
+ * ( integral | replication | union | call | reference )
+ * </pre>
+ *
+ * @return next LayoutElement from the stream or {@code null} if end of
stream is reached.
+ * @throws Pack200Exception If the layout definition is invalid.
+ */
+ public LAYOUT_ELEMENT readLayoutElement() throws Pack200Exception {
+ if (eof()) {
+ return null;
+ }
+ try {
+ switch (peek()) {
+ // Integrals
+ case 'B':
+ case 'H':
+ case 'I':
+ case 'V':
+ case 'S':
+ case 'F':
+ case 'O':
+ case 'P':
+ return factory.createIntegral(readIntegralTag());
+ // Replication
+ case 'N': {
+ next();
+ final String integral = readUnsignedInt();
+ final String body = readBody();
+ if (body.isEmpty()) {
+ throw new Pack200Exception("Corrupted Pack200 archive:
Replication body cannot be empty.");
+ }
+ return factory.createReplication(integral, body);
+ }
+ // Union
+ case 'T': {
+ next();
+ final String anyInt = readAnyInt();
+ final List<UnionCaseData> unionCases = new ArrayList<>();
+ UnionCaseData data;
+ while ((data = readUnionCase()) != null) {
+ unionCases.add(data);
+ }
+ expect('(');
+ expect(')');
+ final String body = readBody();
+ return factory.createUnion(anyInt, unionCases, body);
+ }
+ // Call
+ case '(': {
+ next();
+ final int callNumber = readNumeral();
+ expect(')');
+ return factory.createCall(callNumber);
+ }
+ // Reference
+ case 'K':
+ case 'R': {
+ return factory.createReference(readReferenceTag());
+ }
+ default: {
+ throw new Pack200Exception("Unexpected character '" +
peek() + "' in attribute layout definition.");
+ }
+ }
+ } catch (final Exception ex) {
+ throw new Pack200Exception(String.format("Corrupted Pack200
archive: Invalid layout '%s' at position %d.", definition, p), ex);
+ }
+ }
+
+ /**
+ * Reads a number from the stream.
+ *
+ * <p>Stops reading at the first character, which is not a digit.</p>
+ *
+ * <p><strong>Note:</strong> there is a typo in the
+ * <a
href="https://docs.oracle.com/en/java/javase/11/docs/specs/pack-spec.html#attribute-layout-definitions";>official
{@code numeral} definition</a>.
+ * Parentheses should <strong>not</strong> be part of the numeral
definition.</p>
+ *
+ * <pre>
+ * numeral:
+ * ('-')? (digit)+
+ * </pre>
+ *
+ * @return A number from the stream.
+ * @throws IOException If an I/O error occurs.
+ */
+ private int readNumeral() throws IOException {
+ // Determine if the number is negative
+ final char first = peek();
+ final boolean negative = first == '-';
+ if (negative) {
+ next();
+ }
+ // Read the number
+ final char firstDigit = next();
+ if (!Character.isDigit(firstDigit)) {
+ throw new Pack200Exception("Corrupted Pack200 archive: numeral
must start with a digit.");
+ }
+ long result = firstDigit - '0';
+ while (!eof()) {
+ if (!Character.isDigit(peek())) {
+ break;
+ }
+ result = result * 10 + next() - '0';
+ // Check for overflow
+ if (result > Integer.MAX_VALUE) {
+ throw new Pack200Exception("Corrupted Pack200 archive: numeral
value out of range.");
+ }
+ }
+ return (int) (negative ? -result : result);
+ }
+
+ private String readReferenceTag() throws Pack200Exception {
+ final char[] buf = new char[4];
+ int len = 0;
+ buf[len++] = next();
+ buf[len++] = next();
+ char c = next();
+ buf[len++] = c;
+ if (c == 'N') {
+ c = next();
+ buf[len++] = c;
+ }
+ return AttributeLayoutUtils.checkReferenceTag(new String(buf, 0, len));
+ }
+
+ /**
+ * Reads a non default {@code union_case} from the stream
+ *
+ * <p>Reads a non default {@code union_case} or returns {@code null} if
the default case {@code ()} is encountered.</p>
+ *
+ * <pre>
+ * union_case:
+ * '(' union_case_tag (',' union_case_tag)* ')' '[' (body)? ']'
+ * union_case_tag:
+ * ( numeral | numeral '-' numeral )
+ * </pre>
+ *
+ * @return A UnionCase from the stream or {@code null} if the default case
is encountered.
+ * @throws IOException If an I/O error occurs.
Review Comment:
The @throws annotation mentions `IOException` but the method doesn't declare
throwing `IOException` in its signature. The method signature should throw
`Pack200Exception` based on the method's behavior (calling methods that throw
Pack200Exception and using Pack200Exception in error handling).
##########
src/main/java/org/apache/commons/compress/harmony/internal/AttributeLayoutUtils.java:
##########
@@ -0,0 +1,252 @@
+/*
+ * 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
+ *
+ * https://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.apache.commons.compress.harmony.internal;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import org.apache.commons.compress.harmony.pack200.Pack200Exception;
+import org.apache.commons.lang3.Range;
+
+public final class AttributeLayoutUtils {
+
+ /**
+ * <pre>
+ * integral:
+ * ( unsigned_int | signed_int | bc_index | bc_offset | flag )
+ * signed_int:
+ * 'S' uint_type
+ * any_int:
+ * ( unsigned_int | signed_int )
+ * bc_index:
+ * ( 'P' uint_type | 'PO' uint_type )
+ * bc_offset:
+ * 'O' any_int
+ * flag:
+ * 'F' uint_type
+ * uint_type:
+ * ( 'B' | 'H' | 'I' | 'V' )
+ * </pre>
+ */
+ private static final Set<String> INTEGRAL_TAGS;
+
+ /**
+ * <pre>
+ * reference:
+ * reference_type ( 'N' )? uint_type
+ * reference_type:
+ * ( constant_ref | schema_ref | utf8_ref | untyped_ref )
+ * constant_ref:
+ * ( 'KI' | 'KJ' | 'KF' | 'KD' | 'KS' | 'KQ' | 'KM' | 'KT' | 'KL' )
+ * schema_ref:
+ * ( 'RC' | 'RS' | 'RD' | 'RF' | 'RM' | 'RI' | 'RY' | 'RB' | 'RN' )
+ * utf8_ref:
+ * 'RU'
+ * untyped_ref:
+ * 'RQ'
+ * uint_type:
+ * ( 'B' | 'H' | 'I' | 'V' )
+ * </pre>
+ */
+ private static final Set<String> REFERENCE_TAGS;
+
+ /**
+ * <pre>
+ * uint_type:
+ * ( 'B' | 'H' | 'I' | 'V' )
+ * </pre>
+ */
+ private static final Set<String> UNSIGNED_INT_TAGS;
+
+ static {
+ final Set<String> unsignedIntTags = new HashSet<>();
+ Collections.addAll(unsignedIntTags,
+ // unsigned_int
+ "B", "H", "I", "V");
+ UNSIGNED_INT_TAGS = Collections.unmodifiableSet(unsignedIntTags);
+ final Set<String> integralTags = new HashSet<>();
+ UNSIGNED_INT_TAGS.forEach(tag -> Collections.addAll(integralTags,
+ // unsigned_int
+ tag,
+ // signed_int
+ "S" + tag,
+ // bc_index
+ "P" + tag, "PO" + tag,
+ // bc_offset
+ "O" + tag, "OS" + tag,
+ // flag
+ "F" + tag)); INTEGRAL_TAGS =
Collections.unmodifiableSet(integralTags);
Review Comment:
The statement `INTEGRAL_TAGS = Collections.unmodifiableSet(integralTags);`
is placed on the same line as the closing of the `forEach` lambda, making it
difficult to read. Consider moving it to a separate line for better clarity.
```suggestion
"F" + tag));
INTEGRAL_TAGS = Collections.unmodifiableSet(integralTags);
```
##########
src/main/java/org/apache/commons/compress/harmony/unpack200/NewAttributeBands.java:
##########
@@ -129,7 +168,16 @@ public static class Callable implements
AttributeLayoutElement {
private int index;
- public Callable(final List<LayoutElement> body) {
+ /**
+ * Constructs a new Callable layout element with the given body.
+ *
+ * @param body the body of the callable.
+ * @throws Pack200Exception If the body is empty.
+ */
+ public Callable(final List<LayoutElement> body) throws
Pack200Exception {
Review Comment:
Typo in the method signature: there's an extra space between `throws` and
`Pack200Exception`. It should be `throws Pack200Exception` with a single space.
```suggestion
public Callable(final List<LayoutElement> body) throws
Pack200Exception {
```
##########
src/main/java/org/apache/commons/compress/harmony/internal/AttributeLayoutParser.java:
##########
@@ -0,0 +1,362 @@
+/*
+ * 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
+ *
+ * https://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.apache.commons.compress.harmony.internal;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+
+import org.apache.commons.compress.harmony.pack200.Pack200Exception;
+import org.apache.commons.lang3.Range;
+
+/**
+ * Encapsulates parsing of attribute layout definitions.
+ *
+ * <p>Previously the parsing logic was duplicated in {@link
org.apache.commons.compress.harmony.pack200.NewAttributeBands} and
+ * {@link org.apache.commons.compress.harmony.unpack200.NewAttributeBands}.</p>
+ *
+ * @param <LAYOUT_ELEMENT> the type corresponding to the {@code
layout_element} production
+ * @param <ATTRIBUTE_LAYOUT_ELEMENT> the type corresponding to the elements of
the {@code attribute_layout} production: either {@code layout_element} or
+ * {@code callable}
+ */
+public final class AttributeLayoutParser<ATTRIBUTE_LAYOUT_ELEMENT,
LAYOUT_ELEMENT extends ATTRIBUTE_LAYOUT_ELEMENT> {
+
+ /**
+ * Factory interface for creating attribute layout elements.
+ */
+ public interface Factory<ATTRIBUTE_LAYOUT_ELEMENT, LAYOUT_ELEMENT extends
ATTRIBUTE_LAYOUT_ELEMENT> {
+ LAYOUT_ELEMENT createCall(int callableIndex);
+
+ ATTRIBUTE_LAYOUT_ELEMENT createCallable(String body) throws
Pack200Exception;
+
+ LAYOUT_ELEMENT createIntegral(String tag);
+
+ LAYOUT_ELEMENT createReference(String tag);
+
+ LAYOUT_ELEMENT createReplication(String unsignedInt, String body)
throws Pack200Exception;
+
+ LAYOUT_ELEMENT createUnion(String anyInt, List<UnionCaseData> cases,
String body) throws Pack200Exception;
+ }
+
+ /**
+ * Data class representing a union case in an attribute layout definition.
+ */
+ public static final class UnionCaseData {
+ public final String body;
+ public final List<Range<Integer>> tagRanges;
+
+ private UnionCaseData(final List<Range<Integer>> tagRanges, final
String body) {
+ this.tagRanges = Collections.unmodifiableList(tagRanges);
+ this.body = body;
+ }
+ }
+
+ private final CharSequence definition;
+ private final Factory<ATTRIBUTE_LAYOUT_ELEMENT, LAYOUT_ELEMENT> factory;
+ private int p;
+
+ public AttributeLayoutParser(final CharSequence definition, final
Factory<ATTRIBUTE_LAYOUT_ELEMENT, LAYOUT_ELEMENT> factory) {
+ this.definition = definition;
+ this.factory = factory;
+ }
+
+ private void ensureNotEof() throws Pack200Exception {
+ if (eof()) {
+ throw new Pack200Exception("Incomplete attribute layout
definition: " + definition);
+ }
+ }
+
+ private boolean eof() {
+ return p >= definition.length();
+ }
+
+ private char expect(char... expected) throws Pack200Exception {
+ final char c = next();
+ for (char e : expected) {
+ if (c == e) {
+ return c;
+ }
+ }
+ throw new Pack200Exception("Invalid attribute layout definition:
expected one of " + new String(expected) + ", found '" + c + "'");
+ }
+
+ private char next() throws Pack200Exception {
+ ensureNotEof();
+ return definition.charAt(p++);
+ }
+
+ private char peek() throws Pack200Exception {
+ ensureNotEof();
+ return definition.charAt(p);
+ }
+
+ private String readAnyInt() throws Pack200Exception {
+ final char first = next();
+ return AttributeLayoutUtils.checkAnyIntTag(first == 'S' ? "S" + next()
: String.valueOf(first));
+ }
+
+ /**
+ * Reads the next {@code attribute_layout_element} from the stream.
+ *
+ * <pre>
+ * attribute_layout_element:
+ * ( callable | layout_element )
+ * callable:
+ * '[' (body)? ']'
+ * </pre>
+ *
+ * @return next AttributeLayoutElement from the stream or {@code null} if
end of stream is reached.
+ * @throws Pack200Exception If the layout definition is invalid.
+ */
+ public ATTRIBUTE_LAYOUT_ELEMENT readAttributeLayoutElement() throws
Pack200Exception {
+ if (eof()) {
+ return null;
+ }
+ final char first = peek();
+ if (first == '[') {
+ try {
+ final String body = readBody();
+ if (body.isEmpty()) {
+ throw new Pack200Exception("Corrupted Pack200 archive:
Callable body cannot be empty.");
+ }
+ return factory.createCallable(body);
+ } catch (final Exception ex) {
+ throw new Pack200Exception(String.format("Corrupted Pack200
archive: Invalid layout '%s' at position %d.", definition, p), ex);
+ }
+ }
+ return readLayoutElement();
+ }
+
+ private String readBody() throws Pack200Exception {
+ expect('[');
+ int depth = 1;
+ final StringBuilder body = new StringBuilder();
+ char c;
+ while (true) {
+ c = next();
+ if (c == '[') {
+ depth++;
+ } else if (c == ']') {
+ depth--;
+ if (depth == 0) {
+ break;
+ }
+ }
+ body.append(c);
+ }
+ return body.toString();
+ }
+
+ private String readIntegralTag() throws Pack200Exception {
+ char c = next();
+ final char[] buf = new char[3];
+ int len = 0;
+ buf[len++] = c;
+ if (c == 'F' || c == 'O' || c == 'P' || c == 'S') {
+ c = next();
+ buf[len++] = c;
+ if (c == 'O' || c == 'S') {
+ c = next();
+ buf[len++] = c;
+ }
+ }
+ return AttributeLayoutUtils.checkIntegralTag(new String(buf, 0, len));
+ }
+
+ /**
+ * Reads the next {@code layout_element} from the stream.
+ *
+ * <pre>
+ * layout_element:
+ * ( integral | replication | union | call | reference )
+ * </pre>
+ *
+ * @return next LayoutElement from the stream or {@code null} if end of
stream is reached.
+ * @throws Pack200Exception If the layout definition is invalid.
+ */
+ public LAYOUT_ELEMENT readLayoutElement() throws Pack200Exception {
+ if (eof()) {
+ return null;
+ }
+ try {
+ switch (peek()) {
+ // Integrals
+ case 'B':
+ case 'H':
+ case 'I':
+ case 'V':
+ case 'S':
+ case 'F':
+ case 'O':
+ case 'P':
+ return factory.createIntegral(readIntegralTag());
+ // Replication
+ case 'N': {
+ next();
+ final String integral = readUnsignedInt();
+ final String body = readBody();
+ if (body.isEmpty()) {
+ throw new Pack200Exception("Corrupted Pack200 archive:
Replication body cannot be empty.");
+ }
+ return factory.createReplication(integral, body);
+ }
+ // Union
+ case 'T': {
+ next();
+ final String anyInt = readAnyInt();
+ final List<UnionCaseData> unionCases = new ArrayList<>();
+ UnionCaseData data;
+ while ((data = readUnionCase()) != null) {
+ unionCases.add(data);
+ }
+ expect('(');
+ expect(')');
+ final String body = readBody();
+ return factory.createUnion(anyInt, unionCases, body);
+ }
+ // Call
+ case '(': {
+ next();
+ final int callNumber = readNumeral();
+ expect(')');
+ return factory.createCall(callNumber);
+ }
+ // Reference
+ case 'K':
+ case 'R': {
+ return factory.createReference(readReferenceTag());
+ }
+ default: {
+ throw new Pack200Exception("Unexpected character '" +
peek() + "' in attribute layout definition.");
+ }
+ }
+ } catch (final Exception ex) {
+ throw new Pack200Exception(String.format("Corrupted Pack200
archive: Invalid layout '%s' at position %d.", definition, p), ex);
+ }
+ }
+
+ /**
+ * Reads a number from the stream.
+ *
+ * <p>Stops reading at the first character, which is not a digit.</p>
+ *
+ * <p><strong>Note:</strong> there is a typo in the
+ * <a
href="https://docs.oracle.com/en/java/javase/11/docs/specs/pack-spec.html#attribute-layout-definitions";>official
{@code numeral} definition</a>.
+ * Parentheses should <strong>not</strong> be part of the numeral
definition.</p>
+ *
+ * <pre>
+ * numeral:
+ * ('-')? (digit)+
+ * </pre>
+ *
+ * @return A number from the stream.
+ * @throws IOException If an I/O error occurs.
Review Comment:
The @throws annotation mentions `IOException` but the method doesn't declare
throwing `IOException` in its signature. The method signature shows it throws
`Pack200Exception` (as seen in the catch block at line 282).
```suggestion
```
##########
src/main/java/org/apache/commons/compress/harmony/internal/AttributeLayoutUtils.java:
##########
@@ -0,0 +1,252 @@
+/*
+ * 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
+ *
+ * https://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.apache.commons.compress.harmony.internal;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import org.apache.commons.compress.harmony.pack200.Pack200Exception;
+import org.apache.commons.lang3.Range;
+
+public final class AttributeLayoutUtils {
+
+ /**
+ * <pre>
+ * integral:
+ * ( unsigned_int | signed_int | bc_index | bc_offset | flag )
+ * signed_int:
+ * 'S' uint_type
+ * any_int:
+ * ( unsigned_int | signed_int )
+ * bc_index:
+ * ( 'P' uint_type | 'PO' uint_type )
+ * bc_offset:
+ * 'O' any_int
+ * flag:
+ * 'F' uint_type
+ * uint_type:
+ * ( 'B' | 'H' | 'I' | 'V' )
+ * </pre>
+ */
+ private static final Set<String> INTEGRAL_TAGS;
+
+ /**
+ * <pre>
+ * reference:
+ * reference_type ( 'N' )? uint_type
+ * reference_type:
+ * ( constant_ref | schema_ref | utf8_ref | untyped_ref )
+ * constant_ref:
+ * ( 'KI' | 'KJ' | 'KF' | 'KD' | 'KS' | 'KQ' | 'KM' | 'KT' | 'KL' )
+ * schema_ref:
+ * ( 'RC' | 'RS' | 'RD' | 'RF' | 'RM' | 'RI' | 'RY' | 'RB' | 'RN' )
+ * utf8_ref:
+ * 'RU'
+ * untyped_ref:
+ * 'RQ'
+ * uint_type:
+ * ( 'B' | 'H' | 'I' | 'V' )
+ * </pre>
+ */
+ private static final Set<String> REFERENCE_TAGS;
+
+ /**
+ * <pre>
+ * uint_type:
+ * ( 'B' | 'H' | 'I' | 'V' )
+ * </pre>
+ */
+ private static final Set<String> UNSIGNED_INT_TAGS;
+
+ static {
+ final Set<String> unsignedIntTags = new HashSet<>();
+ Collections.addAll(unsignedIntTags,
+ // unsigned_int
+ "B", "H", "I", "V");
+ UNSIGNED_INT_TAGS = Collections.unmodifiableSet(unsignedIntTags);
+ final Set<String> integralTags = new HashSet<>();
+ UNSIGNED_INT_TAGS.forEach(tag -> Collections.addAll(integralTags,
+ // unsigned_int
+ tag,
+ // signed_int
+ "S" + tag,
+ // bc_index
+ "P" + tag, "PO" + tag,
+ // bc_offset
+ "O" + tag, "OS" + tag,
+ // flag
+ "F" + tag)); INTEGRAL_TAGS =
Collections.unmodifiableSet(integralTags);
+ final Set<String> referenceTags = new HashSet<>();
+ Collections.addAll(referenceTags,
+ // constant_ref
+ "KI", "KJ", "KF", "KD", "KS", "KQ", "KM", "KT", "KL",
+ // schema_ref
+ "RC", "RS", "RD", "RF", "RM", "RI", "RY", "RB", "RN",
+ // utf8_ref
+ "RU",
+ // untyped_ref
+ "RQ");
+ REFERENCE_TAGS = Collections.unmodifiableSet(referenceTags);
+ }
+
+ /**
+ * Validates that the given tag matches the {@code unsigned_int} layout
definition production rule.
+ *
+ * @param tag the layout tag to validate
+ * @return the validated tag
+ * @throws IllegalArgumentException if the tag is invalid
+ */
+ static String checkAnyIntTag(final String tag) {
+ if (UNSIGNED_INT_TAGS.contains(tag) || tag.startsWith("S") &&
UNSIGNED_INT_TAGS.contains(tag.substring(1))) {
+ return tag;
+ }
+ throw new IllegalArgumentException("Invalid unsigned int layout tag: "
+ tag);
+ }
+
+ /**
+ * Validates that the given tag matches the {@code integral} layout
definition production rule.
+ *
+ * @param tag the layout tag to validate
+ * @return the validated tag
+ * @throws IllegalArgumentException if the tag is invalid
+ */
+ public static String checkIntegralTag(final String tag) {
+ if (INTEGRAL_TAGS.contains(tag)) {
+ return tag;
+ }
+ throw new IllegalArgumentException("Invalid integral layout tag: " +
tag);
+ }
+
+ /**
+ * Validates that the given tag matches the {@code reference} layout
definition production rule.
+ *
+ * @param tag the layout tag to validate
+ * @return the validated tag
+ * @throws IllegalArgumentException if the tag is invalid
+ */
+ public static String checkReferenceTag(final String tag) {
+ if (tag.length() >= 3) {
+ final String baseTag = tag.substring(0, 2);
+ final String uintType = tag.substring(tag.length() - 1);
+ if (REFERENCE_TAGS.contains(baseTag) &&
UNSIGNED_INT_TAGS.contains(uintType)
+ && (tag.length() == 3 || tag.length() == 4 &&
tag.charAt(2) == 'N')) {
+ return tag;
+ }
+ }
+ throw new IllegalArgumentException("Invalid reference layout tag: " +
tag);
+ }
+
+ /**
+ * Validates that the given tag matches the {@code unsigned_int} layout
definition production rule.
+ *
+ * @param tag the layout tag to validate
+ * @return the validated tag
+ * @throws IllegalArgumentException if the tag is invalid
+ */
+ static String checkUnsignedIntTag(final String tag) {
+ if (UNSIGNED_INT_TAGS.contains(tag)) {
+ return tag;
+ }
+ throw new IllegalArgumentException("Invalid unsigned int layout tag: "
+ tag);
+ }
+
+ /**
+ * Reads a {@code attribute_layout} from the stream.
+ *
+ * <p>The returned list <strong>may</strong> be empty if the stream is
empty.</p>
+ *
+ * <pre>
+ * attribute_layout:
+ * ( layout_element )* | ( callable )+
+ * </pre>
+ *
+ * @param definition the attribute layout definition body.
+ * @param factory the factory to create AttributeLayoutElements.
+ * @param <ALE>> the type of AttributeLayoutElement.
Review Comment:
Typo in the Javadoc type parameter description: `@param <ALE>>` has an extra
closing angle bracket. It should be `@param <ALE>`.
##########
src/main/java/org/apache/commons/compress/harmony/internal/AttributeLayoutUtils.java:
##########
@@ -0,0 +1,252 @@
+/*
+ * 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
+ *
+ * https://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.apache.commons.compress.harmony.internal;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import org.apache.commons.compress.harmony.pack200.Pack200Exception;
+import org.apache.commons.lang3.Range;
+
+public final class AttributeLayoutUtils {
+
+ /**
+ * <pre>
+ * integral:
+ * ( unsigned_int | signed_int | bc_index | bc_offset | flag )
+ * signed_int:
+ * 'S' uint_type
+ * any_int:
+ * ( unsigned_int | signed_int )
+ * bc_index:
+ * ( 'P' uint_type | 'PO' uint_type )
+ * bc_offset:
+ * 'O' any_int
+ * flag:
+ * 'F' uint_type
+ * uint_type:
+ * ( 'B' | 'H' | 'I' | 'V' )
+ * </pre>
+ */
+ private static final Set<String> INTEGRAL_TAGS;
+
+ /**
+ * <pre>
+ * reference:
+ * reference_type ( 'N' )? uint_type
+ * reference_type:
+ * ( constant_ref | schema_ref | utf8_ref | untyped_ref )
+ * constant_ref:
+ * ( 'KI' | 'KJ' | 'KF' | 'KD' | 'KS' | 'KQ' | 'KM' | 'KT' | 'KL' )
+ * schema_ref:
+ * ( 'RC' | 'RS' | 'RD' | 'RF' | 'RM' | 'RI' | 'RY' | 'RB' | 'RN' )
+ * utf8_ref:
+ * 'RU'
+ * untyped_ref:
+ * 'RQ'
+ * uint_type:
+ * ( 'B' | 'H' | 'I' | 'V' )
+ * </pre>
+ */
+ private static final Set<String> REFERENCE_TAGS;
+
+ /**
+ * <pre>
+ * uint_type:
+ * ( 'B' | 'H' | 'I' | 'V' )
+ * </pre>
+ */
+ private static final Set<String> UNSIGNED_INT_TAGS;
+
+ static {
+ final Set<String> unsignedIntTags = new HashSet<>();
+ Collections.addAll(unsignedIntTags,
+ // unsigned_int
+ "B", "H", "I", "V");
+ UNSIGNED_INT_TAGS = Collections.unmodifiableSet(unsignedIntTags);
+ final Set<String> integralTags = new HashSet<>();
+ UNSIGNED_INT_TAGS.forEach(tag -> Collections.addAll(integralTags,
+ // unsigned_int
+ tag,
+ // signed_int
+ "S" + tag,
+ // bc_index
+ "P" + tag, "PO" + tag,
+ // bc_offset
+ "O" + tag, "OS" + tag,
+ // flag
+ "F" + tag)); INTEGRAL_TAGS =
Collections.unmodifiableSet(integralTags);
+ final Set<String> referenceTags = new HashSet<>();
+ Collections.addAll(referenceTags,
+ // constant_ref
+ "KI", "KJ", "KF", "KD", "KS", "KQ", "KM", "KT", "KL",
+ // schema_ref
+ "RC", "RS", "RD", "RF", "RM", "RI", "RY", "RB", "RN",
+ // utf8_ref
+ "RU",
+ // untyped_ref
+ "RQ");
+ REFERENCE_TAGS = Collections.unmodifiableSet(referenceTags);
+ }
+
+ /**
+ * Validates that the given tag matches the {@code unsigned_int} layout
definition production rule.
+ *
+ * @param tag the layout tag to validate
+ * @return the validated tag
+ * @throws IllegalArgumentException if the tag is invalid
+ */
+ static String checkAnyIntTag(final String tag) {
+ if (UNSIGNED_INT_TAGS.contains(tag) || tag.startsWith("S") &&
UNSIGNED_INT_TAGS.contains(tag.substring(1))) {
+ return tag;
+ }
+ throw new IllegalArgumentException("Invalid unsigned int layout tag: "
+ tag);
+ }
+
+ /**
+ * Validates that the given tag matches the {@code integral} layout
definition production rule.
+ *
+ * @param tag the layout tag to validate
+ * @return the validated tag
+ * @throws IllegalArgumentException if the tag is invalid
+ */
+ public static String checkIntegralTag(final String tag) {
+ if (INTEGRAL_TAGS.contains(tag)) {
+ return tag;
+ }
+ throw new IllegalArgumentException("Invalid integral layout tag: " +
tag);
+ }
+
+ /**
+ * Validates that the given tag matches the {@code reference} layout
definition production rule.
+ *
+ * @param tag the layout tag to validate
+ * @return the validated tag
+ * @throws IllegalArgumentException if the tag is invalid
+ */
+ public static String checkReferenceTag(final String tag) {
+ if (tag.length() >= 3) {
+ final String baseTag = tag.substring(0, 2);
+ final String uintType = tag.substring(tag.length() - 1);
+ if (REFERENCE_TAGS.contains(baseTag) &&
UNSIGNED_INT_TAGS.contains(uintType)
+ && (tag.length() == 3 || tag.length() == 4 &&
tag.charAt(2) == 'N')) {
+ return tag;
+ }
+ }
+ throw new IllegalArgumentException("Invalid reference layout tag: " +
tag);
+ }
+
+ /**
+ * Validates that the given tag matches the {@code unsigned_int} layout
definition production rule.
+ *
+ * @param tag the layout tag to validate
+ * @return the validated tag
+ * @throws IllegalArgumentException if the tag is invalid
+ */
+ static String checkUnsignedIntTag(final String tag) {
+ if (UNSIGNED_INT_TAGS.contains(tag)) {
+ return tag;
+ }
+ throw new IllegalArgumentException("Invalid unsigned int layout tag: "
+ tag);
+ }
+
+ /**
+ * Reads a {@code attribute_layout} from the stream.
+ *
+ * <p>The returned list <strong>may</strong> be empty if the stream is
empty.</p>
+ *
+ * <pre>
+ * attribute_layout:
+ * ( layout_element )* | ( callable )+
+ * </pre>
+ *
+ * @param definition the attribute layout definition body.
+ * @param factory the factory to create AttributeLayoutElements.
+ * @param <ALE>> the type of AttributeLayoutElement.
+ * @param <LE> the type of LayoutElement.
+ * @return not empty list of LayoutElements from the body or, if the
stream is empty, an empty list.
+ * @throws Pack200Exception If the layout definition is invalid.
+ */
+ public static <ALE, LE extends ALE> List<ALE> readAttributeLayout(final
String definition,
+ final AttributeLayoutParser.Factory<ALE, LE> factory) throws
Pack200Exception {
+ final List<ALE> layoutElements = new ArrayList<>();
+ final AttributeLayoutParser<ALE, LE> parser = new
AttributeLayoutParser<>(definition, factory);
+ ALE e;
+ while ((e = parser.readAttributeLayoutElement()) != null) {
+ layoutElements.add(e);
+ }
+ return layoutElements;
+ }
+
+ /**
+ * Reads a {@code body} from the stream.
+ *
+ * <p>The returned list <strong>may</strong> be empty if the stream is
empty.</p>
+ *
+ * <pre>
+ * body:
+ * ( layout_element )+
+ * </pre>
+ *
+ * @param body the attribute layout definition body.
+ * @param factory the factory to create LayoutElements.
+ * @param <ALE>> the type of AttributeLayoutElement.
Review Comment:
Typo in the Javadoc type parameter description: `@param <ALE>>` has an extra
closing angle bracket. It should be `@param <ALE>`.
##########
src/main/java/org/apache/commons/compress/harmony/internal/AttributeLayoutUtils.java:
##########
@@ -0,0 +1,252 @@
+/*
+ * 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
+ *
+ * https://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.apache.commons.compress.harmony.internal;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import org.apache.commons.compress.harmony.pack200.Pack200Exception;
+import org.apache.commons.lang3.Range;
+
+public final class AttributeLayoutUtils {
+
+ /**
+ * <pre>
+ * integral:
+ * ( unsigned_int | signed_int | bc_index | bc_offset | flag )
+ * signed_int:
+ * 'S' uint_type
+ * any_int:
+ * ( unsigned_int | signed_int )
+ * bc_index:
+ * ( 'P' uint_type | 'PO' uint_type )
+ * bc_offset:
+ * 'O' any_int
+ * flag:
+ * 'F' uint_type
+ * uint_type:
+ * ( 'B' | 'H' | 'I' | 'V' )
+ * </pre>
+ */
+ private static final Set<String> INTEGRAL_TAGS;
+
+ /**
+ * <pre>
+ * reference:
+ * reference_type ( 'N' )? uint_type
+ * reference_type:
+ * ( constant_ref | schema_ref | utf8_ref | untyped_ref )
+ * constant_ref:
+ * ( 'KI' | 'KJ' | 'KF' | 'KD' | 'KS' | 'KQ' | 'KM' | 'KT' | 'KL' )
+ * schema_ref:
+ * ( 'RC' | 'RS' | 'RD' | 'RF' | 'RM' | 'RI' | 'RY' | 'RB' | 'RN' )
+ * utf8_ref:
+ * 'RU'
+ * untyped_ref:
+ * 'RQ'
+ * uint_type:
+ * ( 'B' | 'H' | 'I' | 'V' )
+ * </pre>
+ */
+ private static final Set<String> REFERENCE_TAGS;
+
+ /**
+ * <pre>
+ * uint_type:
+ * ( 'B' | 'H' | 'I' | 'V' )
+ * </pre>
+ */
+ private static final Set<String> UNSIGNED_INT_TAGS;
+
+ static {
+ final Set<String> unsignedIntTags = new HashSet<>();
+ Collections.addAll(unsignedIntTags,
+ // unsigned_int
+ "B", "H", "I", "V");
+ UNSIGNED_INT_TAGS = Collections.unmodifiableSet(unsignedIntTags);
+ final Set<String> integralTags = new HashSet<>();
+ UNSIGNED_INT_TAGS.forEach(tag -> Collections.addAll(integralTags,
+ // unsigned_int
+ tag,
+ // signed_int
+ "S" + tag,
+ // bc_index
+ "P" + tag, "PO" + tag,
+ // bc_offset
+ "O" + tag, "OS" + tag,
+ // flag
+ "F" + tag)); INTEGRAL_TAGS =
Collections.unmodifiableSet(integralTags);
+ final Set<String> referenceTags = new HashSet<>();
+ Collections.addAll(referenceTags,
+ // constant_ref
+ "KI", "KJ", "KF", "KD", "KS", "KQ", "KM", "KT", "KL",
+ // schema_ref
+ "RC", "RS", "RD", "RF", "RM", "RI", "RY", "RB", "RN",
+ // utf8_ref
+ "RU",
+ // untyped_ref
+ "RQ");
+ REFERENCE_TAGS = Collections.unmodifiableSet(referenceTags);
+ }
+
+ /**
+ * Validates that the given tag matches the {@code unsigned_int} layout
definition production rule.
+ *
+ * @param tag the layout tag to validate
+ * @return the validated tag
+ * @throws IllegalArgumentException if the tag is invalid
+ */
+ static String checkAnyIntTag(final String tag) {
+ if (UNSIGNED_INT_TAGS.contains(tag) || tag.startsWith("S") &&
UNSIGNED_INT_TAGS.contains(tag.substring(1))) {
+ return tag;
+ }
+ throw new IllegalArgumentException("Invalid unsigned int layout tag: "
+ tag);
Review Comment:
The Javadoc comment says "Validates that the given tag matches the {@code
unsigned_int} layout definition" but the method name is `checkAnyIntTag` which
validates `any_int` (which includes both `unsigned_int` and `signed_int`). The
documentation should say "any_int" instead of "unsigned_int" to match the
method's actual behavior.
##########
src/main/java/org/apache/commons/compress/harmony/unpack200/NewAttributeBands.java:
##########
@@ -553,56 +628,58 @@ public void readBands(final InputStream in, final int
count) throws IOException,
*/
public class UnionCase extends LayoutElement {
- private List<LayoutElement> body;
+ private final List<Range<Integer>> tagRanges;
+ private final List<LayoutElement> body;
- private final List<Integer> tags;
+ public UnionCase(final List<Integer> tags) throws IOException {
Review Comment:
The constructor `UnionCase(final List<Integer> tags) throws IOException`
declares throwing `IOException`, but it only delegates to another constructor
that doesn't throw `IOException`. The `IOException` declaration appears
unnecessary and should be removed from the signature.
```suggestion
public UnionCase(final List<Integer> tags) {
```
--
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]
