Ctf 2 fix draft #367
Ctf 2 fix draft #367
Conversation
WalkthroughBumps bundle version and adds extensive JSON TSdl support: alias expansion, Gson-based metadata parsing, new JSON parsers (static/dynamic strings & arrays, floats, structs), role-aware struct/variant lookups, integer mapping interval-tree, new string/blob declarations, and multiple tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
MatthewKhouzam
force-pushed
the
ctf-2-fix-draft
branch
from
f19df7b to
99a9d62
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Nitpick comments (16)
ctf/org.eclipse.tracecompass.ctf.core/META-INF/MANIFEST.MF (1)
5-5: Version bump to 5.1.0.qualifier looks appropriateGiven the additive nature of the CTF core changes described in this PR, moving from 5.0.x to 5.1.0 is consistent with semantic versioning. Please just ensure:
- Any dependent bundles with tight
[5.0.2,5.1.0)ranges are checked/updated if needed.- Corresponding release notes/changelog entries are aligned with this new minor version.
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/utils/JsonMetadataStrings.java (2)
194-197: Incorrect Javadoc comment for PATH constant.The Javadoc states "Type string for a structure field class" but
PATH = "path"represents a path field location, not a structure type. This appears to be copy-pasted from the STRUCTURE constant below.- /** - * Type string for a structure field class - */ + /** + * Path field within a field-class object + */ public static final String PATH = "path"; //$NON-NLS-1$
240-242: Missing Javadoc comments for MEMBER_CLASSES and MINIMUM_ALIGNMENT.These constants lack Javadoc comments, which is inconsistent with all other public constants in this file.
+ /** + * Member classes of a structure + */ public static final String MEMBER_CLASSES = "member-classes"; //$NON-NLS-1$ + /** + * Minimum alignment of a field class + */ public static final String MINIMUM_ALIGNMENT = "minimum-alignment"; //$NON-NLS-1$ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/BlobDeclaration.java (1)
89-95: Javadoc no longer reflects the implementation.The Javadoc states "Arbitrary decision to have maximum size as 1MB" but the implementation now returns
Math.min(fLength, 1000000), which returns the actual length when under 1MB. Consider updating the documentation to clarify the new semantics./** - * Arbitrary decision to have maximum size as 1MB + * Returns the effective size for this blob, capped at 1MB maximum. */ @Override public int getMaximumSize() { return Math.min(fLength, 1000000); }ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/staticstring/StaticLengthStringParser.java (1)
57-57: Unusedparamparameter.The
paramparameter is declared but never used in theparsemethod. Consider whether it's needed or if it should be used for validation.ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/StringDefinition2.java (1)
34-52: Consider consolidating constructors.Both constructors have identical implementations, differing only in the declaration parameter type. Since
StringDeclarationextendsDeclaration, the more general constructor (lines 48-52) could handle both cases. Consider if theStringDeclaration-specific constructor is necessary for type safety elsewhere.ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/VariantDefinition.java (1)
139-164: Significant code duplication betweenlookupDefinitionoverloads.The two
lookupDefinitionmethods share nearly identical logic. Consider extracting common logic to reduce duplication.Also applies to: 166-192
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/StructDeclaration.java (1)
216-219: Minor style: Consider usingrole != nullfor consistency.The condition
null != role(Yoda condition) is functionally correct but less common in this codebase. Considerrole != nullfor consistency.String role = declaration.getRole(); - if(null != role && !role.isEmpty()) { + if (role != null && !role.isEmpty()) { fRoles.put(role, declaration); }ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/dynamicstring/DynamicLengthStringParser.java (1)
57-57: Unusedparamparameter.The
paramparameter is declared but never used. If theParamclass is intended for future use, consider documenting that; otherwise, this creates unnecessary API surface.ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/types/StaticLengthStringDeclaration.java (3)
70-72: Use integer literal instead of scientific notation.The comparison uses
1e6(floating-point scientific notation) for an integer bound check. Use an integer literal for clarity.Apply this diff:
- if (rawLength > 1e6) { - throw new CTFException("Cannot have a length > 1000000, declared = " + rawLength); //$NON-NLS-1$ + if (rawLength > 1_000_000) { + throw new CTFException("Cannot have a length > 1,000,000, declared = " + rawLength); //$NON-NLS-1$ }
73-77: Optimize byte reading using BitBuffer.get(byte[]).Reading bytes one-by-one in a loop is inefficient. BitBuffer provides a
get(byte[] dst)method that reads multiple bytes at once.Apply this diff:
int length = (int) rawLength; byte[] bytes = new byte[length]; - for (int i = 0; i < length; i++) { - bytes[i] = (byte) input.get(Byte.SIZE, false); - } + input.get(bytes); String value = new String(bytes, fEncoding);
101-105: Incomplete implementation of isBinaryEquivalent.This method returns
falseunconditionally with a TODO comment. If binary equivalence checking is needed for static-length strings, this stub will produce incorrect results.Do you want me to generate a proper implementation or open an issue to track this as a follow-up task?
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/types/DynamicLengthStringDeclaration.java (4)
76-83: Use an integral literal for the max length bound
1e6is a double literal; the comparisonrawLength > 1e6performs an implicit widening conversion to double. It works in practice here but is atypical and may trigger static-analysis warnings.Consider switching to an integral literal:
- if (rawLength > 1e6) { + if (rawLength > 1_000_000L) { throw new CTFException("Cannot have a length > 1000000, declared = " + rawLength); //$NON-NLS-1$ }This keeps the check purely integral and matches the string literal in the error message.
84-88: Optional: bulk-read bytes and guard withcanReadFunctionally, reading each byte via
input.get(Byte.SIZE, false)works, but it is a tight per-byte loop and relies onBitBufferthrowing on out-of-bounds. You can make this both clearer and more efficient by checkingcanReadand using the bulkget(byte[])API.Example refactor:
- byte[] bytes = new byte[length]; - for (int i = 0; i < length; i++) { - bytes[i] = (byte) input.get(Byte.SIZE, false); - } + if (!input.canRead(length * Byte.SIZE)) { + throw new CTFException("Not enough data to read dynamic string of length " + length); //$NON-NLS-1$ + } + byte[] bytes = new byte[length]; + input.get(bytes);This assumes
alignRead(input)is called earlier (as suggested) so thatBitBuffer.get(byte[])starts on a byte boundary.
101-104: Consider aligninggetMaximumSize()with enforced max length
getMaximumSize()returnsInteger.MAX_VALUE, butcreateDefinitionenforcesrawLength <= 1_000_000. This discrepancy may confuse callers that rely ongetMaximumSize()to size buffers or for validation.If
1_000_000bytes is the intended hard cap, consider:
- Either documenting clearly that
getMaximumSize()is intentionally looser than the runtime guard, or- Adjusting
getMaximumSize()to reflect the same bound (in bytes or bits, depending on the contract used elsewhere in the hierarchy).Please cross-check how other dynamic-length declarations implement
getMaximumSize()for consistency.
111-115: ImplementisBinaryEquivalentinstead of always returningfalseLeaving
isBinaryEquivalentas a stub that always returnsfalsemay break reuse of declarations or type caching logic that depends on binary equivalence checks. For declarations with the same encoding and length-field name, it’s reasonable to treat them as equivalent.Possible implementation:
@Override public boolean isBinaryEquivalent(IDeclaration other) { - // TODO Auto-generated method stub - return false; + if (this == other) { + return true; + } + if (!(other instanceof DynamicLengthStringDeclaration)) { + return false; + } + DynamicLengthStringDeclaration o = (DynamicLengthStringDeclaration) other; + return fEncoding.equals(o.fEncoding) && fLengthName.equals(o.fLengthName); }If you prefer to support subclasses or nullable length names, you can relax the equality check accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
ctf/org.eclipse.tracecompass.ctf.core/META-INF/MANIFEST.MF(1 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/metadata/DeclarationScope.java(3 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/BlobDeclaration.java(1 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/ScopedDefinition.java(1 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/StringDefinition2.java(1 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/StructDeclaration.java(5 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/StructDefinition.java(3 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/VariantDeclaration.java(4 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/VariantDefinition.java(3 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/trace/Metadata.java(4 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/TypeAliasParser.java(3 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/dynamicarray/DynamicLengthArrayParser.java(1 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/dynamicstring/DynamicLengthStringParser.java(1 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/staticarray/StaticLengthArrayParser.java(1 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/staticstring/StaticLengthStringParser.java(1 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/struct/StructDeclarationParser.java(1 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/struct/StructParser.java(2 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/variant/VariantParser.java(3 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/types/DynamicLengthStringDeclaration.java(1 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/types/StaticLengthStringDeclaration.java(1 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/utils/JsonMetadataStrings.java(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/TypeAliasParser.java (6)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/dynamicstring/DynamicLengthStringParser.java (1)
DynamicLengthStringParser(46-102)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/enumeration/EnumParser.java (1)
EnumParser(97-266)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/staticarray/StaticLengthArrayParser.java (1)
StaticLengthArrayParser(33-112)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/staticstring/StaticLengthStringParser.java (1)
StaticLengthStringParser(46-102)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/struct/StructParser.java (1)
StructParser(95-285)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/utils/JsonMetadataStrings.java (1)
JsonMetadataStrings(25-244)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/trace/Metadata.java (1)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/utils/JsonMetadataStrings.java (1)
JsonMetadataStrings(25-244)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/struct/StructParser.java (3)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/JsonStructureFieldMemberMetadataNode.java (1)
JsonStructureFieldMemberMetadataNode(31-115)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/JsonStructureFieldMetadataNode.java (1)
JsonStructureFieldMetadataNode(29-89)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/utils/JsonMetadataStrings.java (1)
JsonMetadataStrings(25-244)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/types/DynamicLengthStringDeclaration.java (4)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/io/BitBuffer.java (1)
BitBuffer(32-743)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/Declaration.java (1)
Declaration(30-90)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/IntegerDefinition.java (1)
IntegerDefinition(33-202)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/StringDefinition2.java (1)
StringDefinition2(22-72)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/variant/VariantParser.java (1)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/utils/JsonMetadataStrings.java (1)
JsonMetadataStrings(25-244)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/types/StaticLengthStringDeclaration.java (3)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/io/BitBuffer.java (1)
BitBuffer(32-743)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/Declaration.java (1)
Declaration(30-90)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/StringDefinition2.java (1)
StringDefinition2(22-72)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build / build
- GitHub Check: build / build
🔇 Additional comments (20)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/trace/Metadata.java (2)
38-40: LGTM!The new imports are appropriate for the JSON element-based parsing and alias expansion functionality.
Also applies to: 78-80
268-291: LGTM!Using the pre-parsed and alias-expanded
JsonElementfor allfromJsoncalls is consistent and ensures the expanded aliases are correctly deserialized.ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/struct/StructDeclarationParser.java (1)
131-135: LGTM!The updated logic correctly allows re-registration of the same declaration (idempotent behavior) while still detecting actual conflicts where a different declaration is being registered for the same field name. The null checks ensure safe comparison.
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/metadata/DeclarationScope.java (1)
175-182: LGTM!Good improvements: early return for null/empty names prevents registering anonymous types, and
Objects.equals()provides null-safe comparison for declaration equality.ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/TypeAliasParser.java (1)
172-181: LGTM!The new type handling cases for dynamic/static length strings, arrays, and structures follow the established parsing pattern consistently. Each parser is invoked with the appropriate parameters.
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/struct/StructParser.java (1)
192-200: LGTM!The handling for
JsonStructureFieldMetadataNodecorrectly reads the minimum alignment and member classes, following the established pattern.ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/StructDefinition.java (1)
186-227: LGTM!The exclusion-aware lookup implementation is well-structured:
- Early exit for self-exclusion and null path prevents infinite recursion
- Role-based resolution short-circuits when declaration role matches
- Recursive calls correctly propagate the exclusion parameter
- Parent scope delegation appropriately uses
thisas the exclusionctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/VariantDeclaration.java (1)
127-152: LGTM for the two-branch tag resolution logic.The implementation correctly handles both
IntegerDefinition(with mappings or numeric index) andEnumDefinition(with string value lookup) tag types. Error handling is appropriate with informative messages.ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/StringDefinition2.java (1)
63-66:size()returns character count, not byte count.For strings with multi-byte encodings (e.g., UTF-8),
fString.length()returns the number of characters, not bytes. Ifsize()is expected to return the byte size (consistent with CTF field sizing), this could lead to incorrect calculations.Verify whether
size()should return the byte count based on the string's encoding, or if character count is intentional here.ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/VariantDefinition.java (2)
45-69: LGTM for new constructor supportingSimpleDatatypeDefinition.The new constructor properly initializes the variant definition with the new tag type, enabling support for integer-based variant tags in addition to enum-based ones.
146-151: Potential issue with null role comparison.
getDeclaration().getRole()may returnnull. CallinglookupPath.equals(null)will always returnfalse, which is safe, but the intent may be clearer with a null check. More critically, iffTagDeforfDefinitioncould have a null declaration, this would NPE.Verify that
fTagDef.getDeclaration()andfDefinition.getDeclaration()are guaranteed to be non-null.ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/StructDeclaration.java (3)
59-60: LGTM for role-based lookup mechanism.The
fRolesmap provides an efficient way to look up declarations by their semantic role, which is useful for CTF 2 metadata where roles like "packet-magic-number" or "event-record-id" need to be resolved.
348-352: LGTM for propagating role to self-registration.This ensures that when a struct has a role assigned, it registers itself in the roles map, enabling proper lookup of nested structures by role.
422-430: LGTM for role accessor.The
getRole(String)method provides clean access to child declarations by role.ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/dynamicstring/DynamicLengthStringParser.java (1)
77-101:Paramclass is defined but unused within this parser.The
Paraminner class is defined but theparsemethod doesn't use theparamargument. Verify if this is intentional for API consistency with other parsers or if it should be removed.ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/dynamicarray/DynamicLengthArrayParser.java (2)
78-114: LGTM!The Param inner class is well-structured with proper encapsulation and clear accessors.
72-73: Verify that empty string parameters are safe for element node creation.The JsonStructureFieldMemberMetadataNode is created with empty strings for the name, role, and other parameters. Ensure that TypeAliasParser and downstream code correctly handle these empty values without assuming they contain meaningful identifiers.
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/staticarray/StaticLengthArrayParser.java (2)
75-111: LGTM!The Param inner class follows the same well-structured pattern as in DynamicLengthArrayParser.
69-70: The empty string parameters are safe and intentional.The
JsonStructureFieldMemberMetadataNodecreated with empty strings at lines 69-70 is a synthetic element node for array elements. TypeAliasParser.parse() explicitly handles this case: when the node's type is empty (line 143 of TypeAliasParser), it extracts the actual type from the field class JSON object. This same pattern is identically used in DynamicLengthArrayParser, confirming it is intentional design. No changes needed.ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/types/DynamicLengthStringDeclaration.java (1)
25-94: Overall logic of dynamic string decoding looks solidAside from the points above (nullability, alignment, and minor style/robustness concerns), the core flow—lookup of a length field, validation, bounded read, charset-based decoding, and trimming at the first
'\0'—is clear and matches the intended semantics for a length-prefixed, null-terminated string.
...ecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/metadata/DeclarationScope.java
Outdated
Show resolved
Hide resolved
| public @Nullable IDefinition lookupDefinition(@Nullable String lookupPath, @Nullable ScopedDefinition defintionToExclude) { | ||
| if(defintionToExclude == this) { | ||
| return null; | ||
| } | ||
| return lookupDefinition(lookupPath, defintionToExclude); | ||
| }} |
There was a problem hiding this comment.
Critical: Infinite recursion in lookupDefinition.
Line 169 calls lookupDefinition(lookupPath, defintionToExclude) which is the same method, causing infinite recursion and a StackOverflowError at runtime.
This base implementation should likely delegate to the single-argument lookupDefinition(lookupPath) method, allowing subclasses (like StructDefinition) to override with exclusion-aware behavior.
Apply this diff to fix the recursion:
public @Nullable IDefinition lookupDefinition(@Nullable String lookupPath, @Nullable ScopedDefinition defintionToExclude) {
if(defintionToExclude == this) {
return null;
}
- return lookupDefinition(lookupPath, defintionToExclude);
+ return lookupDefinition(lookupPath);
}
+}
-}}Also fixes the missing newline before the closing brace.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public @Nullable IDefinition lookupDefinition(@Nullable String lookupPath, @Nullable ScopedDefinition defintionToExclude) { | |
| if(defintionToExclude == this) { | |
| return null; | |
| } | |
| return lookupDefinition(lookupPath, defintionToExclude); | |
| }} | |
| public @Nullable IDefinition lookupDefinition(@Nullable String lookupPath, @Nullable ScopedDefinition defintionToExclude) { | |
| if(defintionToExclude == this) { | |
| return null; | |
| } | |
| return lookupDefinition(lookupPath); | |
| } |
🤖 Prompt for AI Agents
In
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/ScopedDefinition.java
around lines 165 to 170, the two-argument lookupDefinition currently calls
itself causing infinite recursion; change it to delegate to the single-argument
lookupDefinition(lookupPath) when defintionToExclude == this so the base
behavior defers to the non-excluding lookup, and ensure there is a newline
before the closing brace.
| */ | ||
| public final class StringDefinition2 extends Definition { | ||
|
|
||
| private final String fString; |
There was a problem hiding this comment.
Potential null pointer exception if value is null.
The value parameter in both constructors lacks @NonNull annotation, and fString is stored directly. If a null value is passed, size() and toString() will throw NPE.
- private final String fString;
+ private final @NonNull String fString;And update constructors:
public StringDefinition2(@NonNull StringDeclaration declaration,
- IDefinitionScope definitionScope, @NonNull String fieldName, String value) {
+ IDefinitionScope definitionScope, @NonNull String fieldName, @NonNull String value) {Also applies to: 35-35, 49-49
🤖 Prompt for AI Agents
In
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/StringDefinition2.java
around lines 24, 35 and 49, the constructors accept a nullable `value` which is
stored in `fString` and later used in size() and toString(), risking NPE; mark
the constructor `value` parameters with @NonNull and add a defensive null check
(e.g. Objects.requireNonNull(value, "value") ) before assigning to fString (or
alternatively normalize null to "" if preferred) so fString can never be null
and size()/toString() are safe.
| private String fTag = null; | ||
| private static final long ALIGNMENT = 1; | ||
| private final Map<String, IDeclaration> fFields = Collections.synchronizedMap(new HashMap<String, IDeclaration>()); | ||
| private IDeclaration[] fFieldArray = new IDeclaration[0]; |
There was a problem hiding this comment.
Thread safety issue with fFieldArray.
fFields is a synchronized map, but fFieldArray is accessed and modified without synchronization. In addField, the array is resized based on fFields.size() and written to without a lock, which can cause race conditions if multiple threads call addField concurrently.
Consider synchronizing access to fFieldArray or making it thread-safe:
public void addField(String fieldTag, IDeclaration declaration) {
+ synchronized (fFields) {
fFields.put(fieldTag, declaration);
int size = fFields.size();
fFieldArray = Arrays.copyOf(fFieldArray, size);
- fFieldArray[size-1]=declaration;
+ fFieldArray[size - 1] = declaration;
+ }
}Additionally, reads of fFieldArray in createDefinition should be synchronized or the field should be marked volatile with proper copy-on-write semantics.
Also applies to: 169-174
🤖 Prompt for AI Agents
In
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/VariantDeclaration.java
around line 49 (and also affecting lines 169-174), fFieldArray is accessed and
mutated without synchronization which can cause race conditions; fix this by
making fFieldArray either a volatile field used with copy-on-write semantics or
by guarding all reads and writes with the same lock used for fFields: wrap the
array resize and assignment in a synchronized block (or use an AtomicReference
and replace the array with a new copied array) so addField atomically replaces
the array and createDefinition reads a stable reference (or mark the array
volatile and ensure you always replace it with a freshly copied array before
assignment).
| if (obj.has(JsonMetadataStrings.TYPE) && obj.get(JsonMetadataStrings.TYPE).getAsString().equals(JsonMetadataStrings.FRAGMENT_FIELD_ALIAS)) { | ||
| metadata.put(obj.get(JsonMetadataStrings.NAME).getAsString(), obj.get(JsonMetadataStrings.FIELD_CLASS).getAsJsonObject()); | ||
| } |
There was a problem hiding this comment.
Add null checks before accessing NAME and FIELD_CLASS properties.
If a malformed FIELD_ALIAS fragment is missing the name or field-class property, get(...).getAsString() or getAsJsonObject() will throw. Consider defensive checks or wrapping in a try-catch.
if (obj.has(JsonMetadataStrings.TYPE) && obj.get(JsonMetadataStrings.TYPE).getAsString().equals(JsonMetadataStrings.FRAGMENT_FIELD_ALIAS)) {
- metadata.put(obj.get(JsonMetadataStrings.NAME).getAsString(), obj.get(JsonMetadataStrings.FIELD_CLASS).getAsJsonObject());
+ JsonElement nameElem = obj.get(JsonMetadataStrings.NAME);
+ JsonElement fieldClassElem = obj.get(JsonMetadataStrings.FIELD_CLASS);
+ if (nameElem != null && nameElem.isJsonPrimitive() && fieldClassElem != null && fieldClassElem.isJsonObject()) {
+ metadata.put(nameElem.getAsString(), fieldClassElem.getAsJsonObject());
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (obj.has(JsonMetadataStrings.TYPE) && obj.get(JsonMetadataStrings.TYPE).getAsString().equals(JsonMetadataStrings.FRAGMENT_FIELD_ALIAS)) { | |
| metadata.put(obj.get(JsonMetadataStrings.NAME).getAsString(), obj.get(JsonMetadataStrings.FIELD_CLASS).getAsJsonObject()); | |
| } | |
| if (obj.has(JsonMetadataStrings.TYPE) && obj.get(JsonMetadataStrings.TYPE).getAsString().equals(JsonMetadataStrings.FRAGMENT_FIELD_ALIAS)) { | |
| JsonElement nameElem = obj.get(JsonMetadataStrings.NAME); | |
| JsonElement fieldClassElem = obj.get(JsonMetadataStrings.FIELD_CLASS); | |
| if (nameElem != null && nameElem.isJsonPrimitive() && fieldClassElem != null && fieldClassElem.isJsonObject()) { | |
| metadata.put(nameElem.getAsString(), fieldClassElem.getAsJsonObject()); | |
| } | |
| } |
🤖 Prompt for AI Agents
In
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/trace/Metadata.java
around lines 263-265, the code assumes the fragment object always contains NAME
and FIELD_CLASS which can cause NPEs or parsing exceptions on malformed
FIELD_ALIAS entries; add defensive checks before accessing those properties
(e.g., verify obj.has(JsonMetadataStrings.NAME) and
obj.has(JsonMetadataStrings.FIELD_CLASS) and that the corresponding JsonElements
are not null and of the expected type) and only call
getAsString()/getAsJsonObject() when these checks pass, or wrap the extraction
in a try-catch to skip/ log malformed entries.
| private final Charset fEncoding; | ||
| private final String fLengthName; | ||
|
|
||
| /** | ||
| * Constructor | ||
| * | ||
| * @param lengthName the name of the length field | ||
| * @param encoding the character encoding | ||
| */ | ||
| public DynamicLengthStringDeclaration(@Nullable String lengthName, Charset encoding) { | ||
| fLengthName = lengthName; | ||
| fEncoding = encoding; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "DynamicLengthStringDeclaration.java" | head -5Repository: eclipse-tracecompass/org.eclipse.tracecompass
Length of output: 223
🏁 Script executed:
find . -type f -name "*.java" -path "*/ctf/*" | grep -i "event/types" | head -10Repository: eclipse-tracecompass/org.eclipse.tracecompass
Length of output: 1193
🏁 Script executed:
cat -n ./ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/types/DynamicLengthStringDeclaration.javaRepository: eclipse-tracecompass/org.eclipse.tracecompass
Length of output: 4814
🏁 Script executed:
rg "new DynamicLengthStringDeclaration" --type java -B 2 -A 2Repository: eclipse-tracecompass/org.eclipse.tracecompass
Length of output: 1064
🏁 Script executed:
cat -n ./ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/dynamicstring/DynamicLengthStringParser.javaRepository: eclipse-tracecompass/org.eclipse.tracecompass
Length of output: 5084
🏁 Script executed:
rg "@NonNull" ./ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/types/ --type java | head -5Repository: eclipse-tracecompass/org.eclipse.tracecompass
Length of output: 1196
Change @Nullable String lengthName to @NonNull String lengthName and add the corresponding import
The constructor parameter lengthName is annotated @Nullable but the field fLengthName and all its usages (in createDefinition at line 68, error message at line 71, and toString at line 108) have no null guards. This creates a contract violation: either the parameter should enforce non-null, or all usages must handle null safely. Since the call site in DynamicLengthStringParser.java (line 70) constructs the string from JSON and never passes null, the parameter should be annotated @NonNull.
Apply the suggested diff and add import org.eclipse.jdt.annotation.NonNull; alongside the existing @Nullable import.
🤖 Prompt for AI Agents
In
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/types/DynamicLengthStringDeclaration.java
around lines 32 to 44, the constructor parameter lengthName is annotated
@Nullable but the field fLengthName and all usages assume non-null; change the
constructor signature to use @NonNull String lengthName and add the import
org.eclipse.jdt.annotation.NonNull alongside the existing @Nullable import so
the API contract matches usage and the code no longer declares a nullable
parameter that is never handled as null.
| @Override | ||
| public StringDefinition2 createDefinition(@Nullable IDefinitionScope definitionScope, String fieldName, BitBuffer input) throws CTFException { | ||
| IDefinition lenDef = null; | ||
| if (definitionScope != null) { | ||
| lenDef = definitionScope.lookupDefinition(fLengthName); | ||
| } | ||
| if (lenDef == null) { | ||
| throw new CTFException("Length field not found: " + fLengthName); //$NON-NLS-1$ | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine DynamicLengthStringDeclaration.java
find . -type f -name "DynamicLengthStringDeclaration.java"Repository: eclipse-tracecompass/org.eclipse.tracecompass
Length of output: 223
🏁 Script executed:
# Search for Declaration subclasses to understand alignment patterns
find . -type f -name "*Declaration.java" -path "*/ctf/core/*" | head -20Repository: eclipse-tracecompass/org.eclipse.tracecompass
Length of output: 2454
🏁 Script executed:
# Look for alignRead usage patterns
rg "alignRead\s*\(" --type java -B 2 -A 2Repository: eclipse-tracecompass/org.eclipse.tracecompass
Length of output: 10973
🏁 Script executed:
# Read the DynamicLengthStringDeclaration.java file
cat -n ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/types/DynamicLengthStringDeclaration.javaRepository: eclipse-tracecompass/org.eclipse.tracecompass
Length of output: 4814
🏁 Script executed:
# Also check StringDeclaration to compare string type implementations
cat -n ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/StringDeclaration.javaRepository: eclipse-tracecompass/org.eclipse.tracecompass
Length of output: 7481
🏁 Script executed:
# Search for tests related to DynamicLengthStringDeclaration
find . -type f -name "*Test*.java" | xargs grep -l "DynamicLengthString" 2>/dev/nullRepository: eclipse-tracecompass/org.eclipse.tracecompass
Length of output: 73
🏁 Script executed:
# Check Declaration base class to understand alignRead
sed -n '1,100p' ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/Declaration.javaRepository: eclipse-tracecompass/org.eclipse.tracecompass
Length of output: 3010
🏁 Script executed:
# Get more context on alignRead usage - check if there are any special cases
rg "createDefinition.*BitBuffer" --type java -A 5 | grep -E "(alignRead|createDefinition)" | head -40Repository: eclipse-tracecompass/org.eclipse.tracecompass
Length of output: 8443
🏁 Script executed:
# Check StaticLengthStringDeclaration to see how it handles alignment
cat -n ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/types/StaticLengthStringDeclaration.javaRepository: eclipse-tracecompass/org.eclipse.tracecompass
Length of output: 4113
Add missing alignRead(input) to respect declared byte alignment
Both DynamicLengthStringDeclaration and StaticLengthStringDeclaration declare byte-level alignment via getAlignment() returning Byte.SIZE, but neither calls alignRead() before reading from the BitBuffer. This breaks parsing when the current bit position is not byte-aligned.
All comparable Declaration types follow this pattern correctly:
StringDeclaration.read()callsalignRead(input)before reading (line 134)IntegerDeclaration,FloatDeclaration,EnumDeclarationall callalignRead()in their read methodsStructDeclaration,VariantDeclaration,ArrayDeclaration, and event header declarations all callalignRead()increateDefinition()
Add alignRead(input); at the start of createDefinition() in both classes before reading from the BitBuffer.
🤖 Prompt for AI Agents
In
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/types/DynamicLengthStringDeclaration.java
around lines 64 to 72, the method createDefinition(...) does not call
alignRead(input) before reading from the BitBuffer; insert a call to
alignRead(input) at the start of createDefinition() (before any lookup/reads) to
enforce byte alignment; apply the same change to
StaticLengthStringDeclaration.createDefinition() as it has the same issue.
| /** | ||
| * Dynamic-length string declaration with encoding support | ||
| * | ||
| * @author Matthew Khouzam | ||
| */ |
There was a problem hiding this comment.
Fix incorrect class javadoc.
The javadoc says "Dynamic-length string declaration" but this class is StaticLengthStringDeclaration for static-length strings.
Apply this diff:
/**
- * Dynamic-length string declaration with encoding support
+ * Static-length string declaration with encoding support
*
* @author Matthew Khouzam
*/📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Dynamic-length string declaration with encoding support | |
| * | |
| * @author Matthew Khouzam | |
| */ | |
| /** | |
| * Static-length string declaration with encoding support | |
| * | |
| * @author Matthew Khouzam | |
| */ |
🤖 Prompt for AI Agents
In
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/types/StaticLengthStringDeclaration.java
around lines 23 to 27, the class Javadoc incorrectly describes the class as a
"Dynamic-length string declaration"; update the Javadoc to correctly describe it
as a "Static-length string declaration" (and keep mention of encoding support
and author unchanged), ensuring the summary accurately reflects the class
purpose.
| /** | ||
| * Constructor | ||
| * | ||
| * @param lengthName | ||
| * the name of the length field | ||
| * @param encoding | ||
| * the character encoding | ||
| */ | ||
| public StaticLengthStringDeclaration(int length, Charset encoding) { | ||
| fLength = length; | ||
| fEncoding = encoding; | ||
| } |
There was a problem hiding this comment.
Fix constructor javadoc parameter mismatch.
The javadoc refers to parameter lengthName of type String, but the actual parameter is length of type int.
Apply this diff:
/**
* Constructor
*
- * @param lengthName
- * the name of the length field
+ * @param length
+ * the fixed length of the string in bytes
* @param encoding
* the character encoding
*/📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Constructor | |
| * | |
| * @param lengthName | |
| * the name of the length field | |
| * @param encoding | |
| * the character encoding | |
| */ | |
| public StaticLengthStringDeclaration(int length, Charset encoding) { | |
| fLength = length; | |
| fEncoding = encoding; | |
| } | |
| /** | |
| * Constructor | |
| * | |
| * @param length | |
| * the fixed length of the string in bytes | |
| * @param encoding | |
| * the character encoding | |
| */ | |
| public StaticLengthStringDeclaration(int length, Charset encoding) { | |
| fLength = length; | |
| fEncoding = encoding; | |
| } |
🤖 Prompt for AI Agents
In
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/types/StaticLengthStringDeclaration.java
around lines 33 to 44, the constructor javadoc incorrectly documents a
non-existent String parameter "lengthName"; update the javadoc to match the
actual constructor signature by replacing the param tag to "@param length the
fixed length of the string (int)" and ensure there is an "@param encoding the
character encoding (Charset)". Keep brief descriptive text for each param and
remove any mention of "lengthName".
| @Override | ||
| public String toString() { | ||
| return "dynamic_string[" + fLength + "]<" + fEncoding.name() + ">"; //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ | ||
| } |
There was a problem hiding this comment.
Correct toString() to indicate static-length string.
The toString() method returns "dynamic_string" but this class represents a static-length string.
Apply this diff:
@Override
public String toString() {
- return "dynamic_string[" + fLength + "]<" + fEncoding.name() + ">"; //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$
+ return "static_string[" + fLength + "]<" + fEncoding.name() + ">"; //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$
}🤖 Prompt for AI Agents
In
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/types/StaticLengthStringDeclaration.java
around lines 96 to 99, the toString() mistakenly labels the instance as
"dynamic_string"; change the returned literal to "static_string" so the method
correctly reflects a static-length string, keeping fLength and fEncoding.name()
as-is and preserving the existing comment markers.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/BlobDeclaration.java (1)
64-76: Fix broken max-size enforcement (currentgetMaximumSize()makes the guard ineffective + risks OOM/DoS).With
getMaximumSize()returningfLength * 8(Line 91),createDefinition()’s check (Line 66) becomes non-protective for non-negative lengths and can also fail viaintoverflow. This allows huge allocations (new byte[fLength], Line 69) from untrusted trace data.Suggested direction (keeps “no arbitrary 1MB” while still preventing runaway allocations):
public class BlobDeclaration extends Declaration { + private static final int DEFAULT_MAX_BLOB_BYTES = + Integer.getInteger("org.eclipse.tracecompass.ctf.maxBlobBytes", 64 * 1024 * 1024); + private final int fLength; private final String fMediaType; @@ public @NonNull BlobDefinition createDefinition(IDefinitionScope definitionScope, @NonNull String fieldName, @NonNull BitBuffer input) throws CTFException { + if (fLength < 0) { + throw new CTFException("Negative blob length: " + fLength); //$NON-NLS-1$ + } if (fLength > getMaximumSize()) { throw new CTFException("Length asked: " + fLength + " is larger than the maximum blob size " + getMaximumSize()); //$NON-NLS-1$ //$NON-NLS-2$ } byte[] array = new byte[fLength]; @@ @Override public int getMaximumSize() { - return fLength*8; + return DEFAULT_MAX_BLOB_BYTES; }(If you don’t want a cap at all, then
getMaximumSize()should not be derived fromfLength, andcreateDefinition()should still hard-fail on negative lengths to avoidNegativeArraySizeException.)Also applies to: 89-92
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/IntegerDeclaration.java (1)
566-577: Critical:hashCode()andequals()contract violation.The
equals()method now includesfMappingsandgetRole()in equality checks (Lines 603-608), buthashCode()(Lines 566-577) does not include these fields. This violates the Java contract: equal objects must have equal hash codes.Objects with identical base properties but different mappings/roles will have the same hash code but not be equal, causing incorrect behavior in hash-based collections like
HashMapandHashSet.Add
fMappingsand role tohashCode():@Override public int hashCode() { final int prime = 31; int result = 1; result = prime * result + (int) (fAlignment ^ (fAlignment >>> 32)); result = prime * result + fBase; result = prime * result + fByteOrder.toString().hashCode(); result = prime * result + fClock.hashCode(); result = prime * result + fEncoding.hashCode(); result = prime * result + fLength; result = prime * result + (fSigned ? 1231 : 1237); + result = prime * result + Objects.hashCode(fMappings); + result = prime * result + Objects.hashCode(getRole()); return result; }Also applies to: 603-608
🧹 Nitpick comments (4)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/metadata/DeclarationScope.java (2)
172-186:registerType: good idempotency + anonymous-type skip; consider enforcing non-null input contract
Skippingnull/empty names (Line 175) avoids polluting the registry with anonymous types, and theObjects.equalscomparison (Line 180) enables “same type, re-register is OK” semantics. Main watch-out: this behavior depends onIDeclaration.equals()being well-defined (structural vs reference).
198-206:registerIdentifier: add a guard for null/empty identifier names
UnlikeregisterType,registerIdentifierwill currently acceptname == nullorname.isEmpty(), which permitsnullkeys inHashMapand can cause hard-to-debug overwrites if the parser ever produces an unnamed identifier. Suggest throwingParseException(orIllegalArgumentException) early.public void registerIdentifier(String name, IDeclaration declaration) throws ParseException { + if (name == null || name.isEmpty()) { + throw new ParseException("Identifier name is null/empty"); //$NON-NLS-1$ + } /* Check if the type has been defined in the current scope */ IDeclaration iDeclaration = fIdentifiers.get(name); if (iDeclaration != null && !Objects.equals(iDeclaration, declaration)) { throw new ParseException("Identifier has already been defined:" + name); //$NON-NLS-1$ } /* Add it to the register. */ fIdentifiers.put(name, declaration); }ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/BlobDeclaration.java (1)
35-62: Clarify whetherlenis “bits” or “bytes” (Javadoc vs implementation mismatch).Constructor docs say “length in bits” (Line 39, 51) but parsing/array allocation treats
fLengthas bytes (Line 69-73). This will confuse future changes and makes it hard to reason about size limits.ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/StructDefinition.java (1)
188-190: Prefer==for identity comparison.Using
this.equals(defintionToExclude)works but==is more idiomatic and efficient for checking object identity (i.e., same instance). Theequalsmethod may be overridden to compare content, which is not the intent here.- if (this.equals(defintionToExclude) || lookupPath == null) { + if (this == defintionToExclude || lookupPath == null) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/metadata/DeclarationScope.java(3 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/BlobDeclaration.java(1 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/IntegerDeclaration.java(8 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/IntegerDefinition.java(2 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/StructDefinition.java(4 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/IOStructGen.java(3 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/JsonTraceMetadataNode.java(3 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/integer/IntegerDeclarationParser.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
🔇 Additional comments (11)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/metadata/DeclarationScope.java (1)
20-20: Import addition is fine; keep an eye onequals()coverage
java.util.Objectsis now load-bearing for duplicate detection semantics; if anyIDeclarationsubclasses don’t implementequals()appropriately, this still degenerates to reference-identity checks.ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/IntegerDefinition.java (1)
109-114: Lazy initialization is not thread-safe, but impact is minimal.Multiple threads calling
getMappings()concurrently could compute the mapping redundantly. SinceStringassignment is atomic and the computation is deterministic, the worst outcome is wasted computation, not incorrect behavior. Consider usingvolatileor a synchronized block if this is called frequently in multi-threaded contexts.ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/integer/IntegerDeclarationParser.java (1)
264-269: Good fix: Corrected parameter order for mapped declarations.The first argument to
createDeclarationis now correctly(int) sizeinstead ofbase, aligning the mapped path (Line 265) with the non-mapped path (Line 268). This fixes a bug where mapped integer declarations would have been created with incorrect bit lengths.ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/IntegerDeclaration.java (2)
549-551: Good improvement: UsingLong.SIZEconstant.Replacing the hardcoded
64withLong.SIZEimproves readability and self-documents the intent.
134-146: Interval tree structure is appropriate for the mapping use case.The
IntervalNodeinner class and sorted list approach provide a reasonable foundation for mapping lookups. Note that the algorithm using this structure has issues (addressed separately).Also applies to: 372-387
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/IOStructGen.java (2)
23-26: LGTM!The new imports are necessary for the environment propagation logic and are used appropriately.
Also applies to: 52-52
161-162: The current implementation is correct. Environment values from JSON traces are intentionally stored in their JSON representation format (usingtoString()), and downstream code inLttngUstTraceandLttngKernelTraceexplicitly validates against this format by checking for literal"\"ust\""and"\"kernel\""strings with escaped quotes included.Likely an incorrect or invalid review comment.
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/JsonTraceMetadataNode.java (1)
19-19: Library migration is complete and properly configured.The migration from
org.json.JSONObjecttocom.google.gson.JsonObjectis consistent across import, field declaration, and getter return type. The@SerializedNameannotation is properly supported by Gson, and the dependency is correctly declared inMETA-INF/MANIFEST.MFunderImport-Package. All references in the codebase have been updated, including the usage inIOStructGen.javawheregetEnvironment().entrySet()is called.ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/StructDefinition.java (3)
205-219: Role-based lookup enhancement looks correct.The logic properly short-circuits when a child's declaration role matches the lookup path, avoiding unnecessary recursion. The exclusion propagation via
defintionToExcludecorrectly prevents infinite loops in cyclic references.
238-252: Null-safety and recursive traversal additions are correct.The early return for null role and the delegation to
lookupRoleRecursivefor nested structures follow a sound pattern.
254-266: Add null check before accessinggetCurrentField()result to prevent NullPointerException.On line 259,
getCurrentField()may return null if no field is currently selected. Accessingcurrent.getDeclaration()on line 260 without a null check can cause NPE. Add a guard before dereferencing:private IDefinition lookupRoleRecursive(IDefinition def, String role) { if (def instanceof StructDefinition) { return ((StructDefinition) def).lookupRole(role); } if (def instanceof VariantDefinition) { IDefinition current = ((VariantDefinition) def).getCurrentField(); + if (current == null) { + return null; + } if (role.equals(current.getDeclaration().getRole())) { return current; } return lookupRoleRecursive(current, role); } return null; }
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ctf/org.eclipse.tracecompass.tmf.ctf.core/src/org/eclipse/tracecompass/tmf/ctf/core/event/CtfTmfEventField.java (1)
121-130: Good:StringDefinition2is now parsed as a TMF string; consider null-safety + dedup.
- This mirrors the
StringDefinitionpath correctly.- Suggestion: defensively handle a potential
nullvalue (e.g., map to"") or enforce@NonNullinStringDefinition2(itssize()currently assumes non-null).- Optional: collapse the two branches to reduce duplication (e.g., helper
parseStringDefinition(...)/ common interface).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ctf/org.eclipse.tracecompass.tmf.ctf.core/src/org/eclipse/tracecompass/tmf/ctf/core/event/CtfTmfEventField.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ctf/org.eclipse.tracecompass.tmf.ctf.core/src/org/eclipse/tracecompass/tmf/ctf/core/event/CtfTmfEventField.java (1)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/StringDefinition2.java (1)
StringDefinition2(22-72)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build / build
- GitHub Check: build / build
🔇 Additional comments (1)
ctf/org.eclipse.tracecompass.tmf.ctf.core/src/org/eclipse/tracecompass/tmf/ctf/core/event/CtfTmfEventField.java (1)
42-43: Import + new type dependency looks fine, but keep the surface area contained.
SinceStringDefinition2is new API in the CTF core, consider whether this file should be the only TMF-side “bridge” for it (vs. sprinklingStringDefinition2checks in multiple places).
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/TypeAliasParser.java (1)
144-148: Potential NullPointerException when accessing type from fieldClass.The call to
fieldClass.get(JsonMetadataStrings.TYPE).getAsString()will throw NPE if theTYPEkey is absent. Additionally, thefieldClass.isJsonObject()check on line 145 is redundant sincefieldClassis already aJsonObject(assigned viagetAsJsonObject()on line 113).Apply this diff to add null safety:
if (type.isEmpty()) { - if (fieldClass.isJsonObject()) { - type = fieldClass.get(JsonMetadataStrings.TYPE).getAsString(); + JsonElement typeElement = fieldClass.get(JsonMetadataStrings.TYPE); + if (typeElement != null && typeElement.isJsonPrimitive()) { + type = typeElement.getAsString(); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/TypeAliasParser.java(3 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/utils/JsonMetadataStrings.java(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
🔇 Additional comments (2)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/TypeAliasParser.java (1)
173-186: LGTM!The new type handling branches follow the established pattern in the file and correctly map each TSdl type to its corresponding parser with appropriate parameters.
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/utils/JsonMetadataStrings.java (1)
50-58: LGTM!The new constants are properly documented and follow the established naming conventions. The string values align with CTF2 specification terminology.
Also applies to: 160-167, 179-192, 224-247
| /** | ||
| * Type string for a structure field class | ||
| */ | ||
| public static final String PATH = "path"; //$NON-NLS-1$ |
There was a problem hiding this comment.
Incorrect Javadoc for PATH constant.
The Javadoc says "Type string for a structure field class" but this constant represents "path". This appears to be a copy-paste error.
/**
- * Type string for a structure field class
+ * Field string for a path reference
*/
public static final String PATH = "path"; //$NON-NLS-1$📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Type string for a structure field class | |
| */ | |
| public static final String PATH = "path"; //$NON-NLS-1$ | |
| /** | |
| * Field string for a path reference | |
| */ | |
| public static final String PATH = "path"; //$NON-NLS-1$ |
🤖 Prompt for AI Agents
In
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/utils/JsonMetadataStrings.java
around lines 203 to 206, the Javadoc on the PATH constant is incorrect (it
currently reads "Type string for a structure field class"); update the Javadoc
to accurately describe the constant (e.g., "Key for the path string in JSON
metadata" or "Path key used in JSON metadata") so it reflects that the constant
represents "path" rather than a structure field type.
| public static final Map<String, Charset> ENCODINGS = Map.of("utf-8",StandardCharsets.UTF_8, //$NON-NLS-1$ | ||
| "utf-16be",StandardCharsets.UTF_16BE,"utf-16le",StandardCharsets.UTF_16LE, //$NON-NLS-1$ //$NON-NLS-2$ | ||
| "utf-32be", Charset.forName("UTF-32BE"),"utf-32le", Charset.forName("UTF-32LE")); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$ |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
UTF-32BE UTF-32LE charset Java JDK standard availability
💡 Result:
Short answer: Modern JDKs include UTF-32BE/UTF-32LE as standard charsets (and as StandardCharsets constants). They were added around JDK 22; older JDK releases (e.g., JDK 13) did not list UTF-32 charsets as standard. [1][2][3]
Sources:
- Java Charset spec (shows UTF-32BE/UTF-32LE in the list of standard charsets). [1]
- StandardCharsets API (shows UTF_32, UTF_32BE, UTF_32LE constants; notes "Since: 22"). [2]
- OpenJDK bug/CSR tracking the addition of UTF-32 charsets (JDK-8310047 — resolved for JDK 22). [3]
UTF-32 charset handling should account for JDK version compatibility.
Charset.forName("UTF-32BE") and Charset.forName("UTF-32LE") are problematic on JDKs prior to version 22. UTF-32 charsets were added to StandardCharsets only in JDK 22; on earlier versions, Charset.forName() will throw UnsupportedCharsetException at class loading time.
If the project targets JDK 22+, replace with StandardCharsets.UTF_32BE and StandardCharsets.UTF_32LE. If older JDK versions must be supported, use lazy initialization with Charset.isSupported() checks to gracefully handle unavailable charsets.
🤖 Prompt for AI Agents
In
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/utils/JsonMetadataStrings.java
around lines 220-222, the static ENCODINGS map uses Charset.forName("UTF-32BE")
and Charset.forName("UTF-32LE"), which can throw UnsupportedCharsetException at
class load time on JDKs <22; if this project targets JDK 22+, replace those
entries with StandardCharsets.UTF_32BE and StandardCharsets.UTF_32LE; otherwise,
change the static initialization to a safe, lazy or guarded approach (e.g.
populate the map in a static block or factory method that checks
Charset.isSupported(...) or catches UnsupportedCharsetException before calling
Charset.forName, and only include UTF-32 entries when supported) so class
loading never fails.
| public static final String MEMBER_CLASSES = "member-classes"; //$NON-NLS-1$ | ||
|
|
||
| public static final String MINIMUM_ALIGNMENT = "minimum-alignment"; //$NON-NLS-1$ |
There was a problem hiding this comment.
Missing Javadoc for public constants.
MEMBER_CLASSES and MINIMUM_ALIGNMENT are missing Javadoc comments, which is inconsistent with the rest of the file.
+ /**
+ * Field string for member classes in a structure
+ */
public static final String MEMBER_CLASSES = "member-classes"; //$NON-NLS-1$
+ /**
+ * Field string for minimum alignment
+ */
public static final String MINIMUM_ALIGNMENT = "minimum-alignment"; //$NON-NLS-1$📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static final String MEMBER_CLASSES = "member-classes"; //$NON-NLS-1$ | |
| public static final String MINIMUM_ALIGNMENT = "minimum-alignment"; //$NON-NLS-1$ | |
| /** | |
| * Field string for member classes in a structure | |
| */ | |
| public static final String MEMBER_CLASSES = "member-classes"; //$NON-NLS-1$ | |
| /** | |
| * Field string for minimum alignment | |
| */ | |
| public static final String MINIMUM_ALIGNMENT = "minimum-alignment"; //$NON-NLS-1$ |
🤖 Prompt for AI Agents
In
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/utils/JsonMetadataStrings.java
around lines 249 to 251, the public constants MEMBER_CLASSES and
MINIMUM_ALIGNMENT are missing Javadoc comments; add concise Javadoc above each
constant matching the style used elsewhere in the file (one-line description,
@since tag if present elsewhere) describing what the JSON key represents (e.g.,
MEMBER_CLASSES — JSON key for member classes list, MINIMUM_ALIGNMENT — JSON key
for minimum alignment value), keep wording and formatting consistent with
existing constant Javadocs.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/utils/JsonMetadataStrings.java (3)
203-206: Incorrect Javadoc for PATH constant.Already flagged in previous review - the Javadoc incorrectly says "Type string for a structure field class" but the constant represents
"path".
220-222: UTF-32 charset JDK compatibility concern.Already flagged in previous review -
Charset.forName("UTF-32BE")andCharset.forName("UTF-32LE")may throwUnsupportedCharsetExceptionon JDKs prior to version 22.
249-251: Missing Javadoc for public constants.Already flagged in previous review -
MEMBER_CLASSESandMINIMUM_ALIGNMENTare missing Javadoc comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/floatingpoint/FloatDeclarationParser.java(2 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/utils/JsonMetadataStrings.java(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
🔇 Additional comments (6)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/utils/JsonMetadataStrings.java (3)
16-19: LGTM!Imports are appropriate for the new
ENCODINGSmap functionality.
50-60: LGTM!Constants are properly documented with Javadoc and follow the established conventions.
160-168: LGTM!Floating-point type constants are properly documented and follow the file conventions.
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/floatingpoint/FloatDeclarationParser.java (3)
25-36: LGTM!Imports are appropriate for the new JSON-based metadata parsing functionality.
133-145: LGTM!The IEEE 754 size mappings are correct:
- 32-bit single precision: exponent=8, mantissa=24
- 64-bit double precision: exponent=11, mantissa=53
Clear error handling for unsupported sizes.
155-192: LGTM!The legacy parsing branch is well-structured with:
- Proper null check for children with clear error message
- Comprehensive handling of all float attributes (exp_dig, byte_order, mant_dig, align)
- Appropriate error handling for unknown attributes
...tracecompass/internal/ctf/core/event/metadata/tsdl/floatingpoint/FloatDeclarationParser.java
Show resolved
Hide resolved
| /** | ||
| * Type string for a null terminated string field class | ||
| */ | ||
| public static final String DYNAMIC_LENGTH_STRING = "dynamic-length-string"; //$NON-NLS-1$ |
There was a problem hiding this comment.
Incorrect Javadoc for DYNAMIC_LENGTH_STRING constant.
The Javadoc says "Type string for a null terminated string field class" but this constant represents "dynamic-length-string". This appears to be a copy-paste error from NULL_TERMINATED_STRING.
/**
- * Type string for a null terminated string field class
+ * Type string for a dynamic length string field class
*/
public static final String DYNAMIC_LENGTH_STRING = "dynamic-length-string"; //$NON-NLS-1$🤖 Prompt for AI Agents
In
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/utils/JsonMetadataStrings.java
around lines 184 to 187, the Javadoc for the DYNAMIC_LENGTH_STRING constant is
incorrect (it describes a null-terminated string); update the Javadoc to
accurately describe that this constant represents a dynamic-length string field
(e.g., "Type string for a dynamic length string field class" or similar),
ensuring the description matches the "dynamic-length-string" value and remove
any reference to null termination.
| public static final String ALIGNMENT = "alignment"; | ||
|
|
||
| public static final String BYTE_ORDER = "byte-order"; |
There was a problem hiding this comment.
Missing Javadoc and externalized string markers.
ALIGNMENT and BYTE_ORDER constants are missing:
- Javadoc comments (inconsistent with the rest of the file)
//$NON-NLS-1$markers for externalized strings
+ /**
+ * Field string for alignment specification
+ */
- public static final String ALIGNMENT = "alignment";
+ public static final String ALIGNMENT = "alignment"; //$NON-NLS-1$
+ /**
+ * Field string for byte order specification
+ */
- public static final String BYTE_ORDER = "byte-order";
+ public static final String BYTE_ORDER = "byte-order"; //$NON-NLS-1$📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static final String ALIGNMENT = "alignment"; | |
| public static final String BYTE_ORDER = "byte-order"; | |
| /** | |
| * Field string for alignment specification | |
| */ | |
| public static final String ALIGNMENT = "alignment"; //$NON-NLS-1$ | |
| /** | |
| * Field string for byte order specification | |
| */ | |
| public static final String BYTE_ORDER = "byte-order"; //$NON-NLS-1$ |
🤖 Prompt for AI Agents
In
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/utils/JsonMetadataStrings.java
around lines 253 to 255, the two constants ALIGNMENT and BYTE_ORDER are missing
Javadoc comments and the //$NON-NLS-1$ externalization markers; add short
Javadoc above each constant consistent with the file style (e.g., describing the
metadata key) and append the //$NON-NLS-1$ marker at the end of each String
declaration to match the rest of the file.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/JsonTraceMetadataNodeTest.java (1)
19-20: Same blocker as above: Gson missing from test classpath.(See comment in
CTF2IntegrationTest.javaabout adding Gson toorg.eclipse.tracecompass.ctf.core.testsdependencies.)Also applies to: 24-58
ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/TypeAliasParserTest.java (1)
24-26: Same blocker as above: Gson missing from test classpath.(See comment in
CTF2IntegrationTest.javaabout adding Gson toorg.eclipse.tracecompass.ctf.core.testsdependencies.)Also applies to: 30-89
🧹 Nitpick comments (4)
ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/CTF2IntegrationTest.java (1)
77-116: Strengthen mapping assertions to validate ranges, not just keys.
testIntegerWithMappings()only checks map keys exist; it won’t catch wrong range parsing. Consider asserting the parsed ranges for"ZERO"and"ONE"are exactly[0,0]and[1,1](whatever the concreteIntegerDeclaration#getMappings()value type is).ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/JsonTraceMetadataNodeTest.java (1)
33-37: Prefer Gson deserialization over reflection to set private fields.These tests are tightly coupled to private field names (
fEnvironment,fUid). If feasible, build JSON and deserialize intoJsonTraceMetadataNodewith Gson to better match production wiring and reduce brittleness.Also applies to: 50-54
ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/TypeAliasParserTest.java (1)
30-52: Tighten assertions to validate parsed properties (not just non-null).For better regression protection, consider asserting key parsed attributes:
- float: expected length/byte-order-derived properties (or explicit exponent/mantissa expected for 32-bit if stable)
- strings: length (static case) and encoding for both static/dynamic cases (if accessible from the returned declaration).
Also applies to: 54-71, 73-89
ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/FloatDeclarationParserTest.java (1)
30-48: Add missing assertion for default alignment in the 32-bit case
FloatDeclarationParsernormalizes missing alignment to1;testCTF2Float32Parsing()should assert it to lock the behavior.assertEquals(ByteOrder.LITTLE_ENDIAN, result.getByteOrder()); + assertEquals(1, result.getAlignment());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/CTF2IntegrationTest.java(1 hunks)ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/FloatDeclarationParserTest.java(1 hunks)ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/JsonTraceMetadataNodeTest.java(1 hunks)ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/TypeAliasParserTest.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/CTF2IntegrationTest.java (4)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/StructDeclaration.java (1)
StructDeclaration(49-432)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/trace/CTFTrace.java (1)
CTFTrace(81-1043)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/JsonStructureFieldMemberMetadataNode.java (1)
JsonStructureFieldMemberMetadataNode(31-115)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/integer/IntegerDeclarationParser.java (1)
IntegerDeclarationParser(81-271)
ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/JsonTraceMetadataNodeTest.java (1)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/JsonTraceMetadataNode.java (1)
JsonTraceMetadataNode(28-87)
ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/TypeAliasParserTest.java (2)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/JsonStructureFieldMemberMetadataNode.java (1)
JsonStructureFieldMemberMetadataNode(31-115)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/TypeAliasParser.java (1)
TypeAliasParser(55-219)
ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/FloatDeclarationParserTest.java (2)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/JsonStructureFieldMemberMetadataNode.java (1)
JsonStructureFieldMemberMetadataNode(31-115)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/floatingpoint/FloatDeclarationParser.java (1)
FloatDeclarationParser(66-206)
🪛 GitHub Actions: Continuous Integration (core tests only)
ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/CTF2IntegrationTest.java
[error] 1-1: Compilation failed: The type com.google.gson.JsonObject cannot be resolved. Imports for Gson classes (JsonArray, JsonObject, JsonPrimitive) are failing, indicating a missing or unresolved Gson dependency during test compilation.
ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/FloatDeclarationParserTest.java
[error] 1-1: Compilation failed: com.google.gson.JsonObject, JsonPrimitive cannot be resolved in tests importing Gson classes.
🪛 GitHub Actions: Continuous Integration (long-ui tests only)
ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/CTF2IntegrationTest.java
[error] 1-1: Compilation failed: The type com.google.gson.JsonObject cannot be resolved. Gson dependencies are missing for CTF tests (CTF2IntegrationTest.java).
[error] 1-1: Compilation failed: Import com.google.gson.JsonArray cannot be resolved in CTF2IntegrationTest.java.
[error] 1-1: Compilation failed: Import com.google.gson.JsonPrimitive cannot be resolved in CTF2IntegrationTest.java.
[error] 1-1: The constructor JsonStructureFieldMemberMetadataNode(...) refers to missing type JsonObject (Gson types not resolved).
ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/JsonTraceMetadataNodeTest.java
[error] 1-1: Compilation failed: The type JsonObject cannot be resolved in JsonTraceMetadataNodeTest.java (imports of Gson types missing).
ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/TypeAliasParserTest.java
[error] 1-1: Compilation failed: Gson-related classes not resolved in TypeAliasParserTest.java.
ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/FloatDeclarationParserTest.java
[error] 1-1: Compilation failed: Gson types (JsonObject/JsonPrimitive) cannot be resolved in FloatDeclarationParserTest.java.
🪛 GitHub Actions: Continuous Integration (short-ui tests only)
ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/CTF2IntegrationTest.java
[error] 1-1: Compilation failure: The type com.google.gson.JsonObject cannot be resolved. It is indirectly referenced from required type org.eclipse.tracecompass.ctf.core.trace.Metadata. Imports for com.google.gson.JsonArray, com.google.gson.JsonObject, and com.google.gson.JsonPrimitive cannot be resolved.
[error] 1-1: Compilation failure: JsonObject cannot be resolved to a type; JsonArray/JsonPrimitive also unresolved in test code.
[error] 1-1: The constructor JsonStructureFieldMemberMetadataNode(ICTFMetadataNode, String, String, String, JsonObject) refers to the missing type JsonObject.
[error] 1-1: CTF2IntegrationTest contains multiple references to Gson Json* classes (JsonObject, JsonArray, JsonPrimitive) that cannot be resolved due to missing Gson dependency in the test classpath.
[error] 1-1: Compilation failure: Unresolved references to JsonObject/JsonPrimitive in test code; likely due to missing Gson on test classpath.
ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/JsonTraceMetadataNodeTest.java
[error] 1-1: Compilation failure: JsonObject and JsonPrimitive types used in tests cannot be resolved due to missing Gson dependency.
ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/TypeAliasParserTest.java
[error] 1-1: Compilation failure: Gson JsonObject/JsonPrimitive types cannot be resolved in TypeAliasParserTest.
ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/FloatDeclarationParserTest.java
[error] 1-1: Compilation failure: The imports for com.google.gson.JsonObject and com.google.gson.JsonPrimitive cannot be resolved.
🔇 Additional comments (1)
ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/CTF2IntegrationTest.java (1)
65-75: Verify API usage:StructDeclaration#getFields()andJsonStructureFieldMemberMetadataNode#setFieldClass(...)may be non-portable.
assertEquals(2, result.getFields().size())andnode.setFieldClass(...)assume APIs that may not exist (or may differ) across versions. IfStructDeclarationdoesn’t exposegetFields(), consider asserting viahasField(...)+ iteratinggetFieldsList()instead. IfJsonStructureFieldMemberMetadataNodedoesn’t support mutation, construct it with the field-class in the constructor instead.
...ss.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/CTF2IntegrationTest.java
Show resolved
Hide resolved
...core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/FloatDeclarationParserTest.java
Show resolved
Hide resolved
| JsonObject fieldClass = new JsonObject(); | ||
| fieldClass.add("length", new JsonPrimitive(32)); | ||
| fieldClass.add("byte-order", new JsonPrimitive("le")); | ||
|
|
||
| JsonStructureFieldMemberMetadataNode node = new JsonStructureFieldMemberMetadataNode(null, "test", "test"); | ||
| node.setFieldClass(fieldClass); | ||
|
|
||
| FloatDeclaration result = FloatDeclarationParser.INSTANCE.parse(node, | ||
| new FloatDeclarationParser.Param(trace)); | ||
|
|
There was a problem hiding this comment.
CI blocker: JsonStructureFieldMemberMetadataNode is constructed/used with the wrong API
The test uses a 3-arg ctor + setFieldClass(...), but the provided class signature takes (parent, type, value, name, fieldClass) and the field class is final (no setter). This will fail compilation even after fixing Gson.
Suggested fix (also drops unnecessary JsonPrimitive usage):
- JsonObject fieldClass = new JsonObject();
- fieldClass.add("length", new JsonPrimitive(32));
- fieldClass.add("byte-order", new JsonPrimitive("le"));
+ JsonObject fieldClass = new JsonObject();
+ fieldClass.addProperty("length", 32);
+ fieldClass.addProperty("byte-order", "le");
- JsonStructureFieldMemberMetadataNode node = new JsonStructureFieldMemberMetadataNode(null, "test", "test");
- node.setFieldClass(fieldClass);
+ JsonStructureFieldMemberMetadataNode node =
+ new JsonStructureFieldMemberMetadataNode(null, "test", "test", "field", fieldClass);
@@
- JsonObject fieldClass = new JsonObject();
- fieldClass.add("length", new JsonPrimitive(64));
- fieldClass.add("byte-order", new JsonPrimitive("be"));
- fieldClass.add("alignment", new JsonPrimitive(8));
+ JsonObject fieldClass = new JsonObject();
+ fieldClass.addProperty("length", 64);
+ fieldClass.addProperty("byte-order", "be");
+ fieldClass.addProperty("alignment", 8);
- JsonStructureFieldMemberMetadataNode node = new JsonStructureFieldMemberMetadataNode(null, "test", "test");
- node.setFieldClass(fieldClass);
+ JsonStructureFieldMemberMetadataNode node =
+ new JsonStructureFieldMemberMetadataNode(null, "test", "test", "field", fieldClass);Also applies to: 54-64
🤖 Prompt for AI Agents
In
ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/FloatDeclarationParserTest.java
around lines 34-43 (and likewise 54-64), the test constructs
JsonStructureFieldMemberMetadataNode with the old 3-arg ctor and calls
setFieldClass(...) which no longer exists; instead call the current constructor
that accepts (parent, type, value, name, fieldClass) and pass the JsonObject
fieldClass as the fifth argument, and simplify the JsonObject contents by adding
plain primitives (e.g., JsonObject.addProperty("length", 32) and
addProperty("byte-order", "le")) rather than wrapping values in JsonPrimitive.
There was a problem hiding this comment.
This is better than claude!
There was a problem hiding this comment.
@MatthewKhouzam, thank you! I'm glad the analysis was helpful. Let me know if you need any clarification on the suggested constructor fix or run into any issues while implementing it. 😊
MatthewKhouzam
force-pushed
the
ctf-2-fix-draft
branch
from
a278a00 to
8cbe6cf
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/FloatDeclarationParserTest.java (1)
38-54: Tests look correct; please double-checkCTFTracelifecycle + Gson OSGi import
The updatedJsonStructureFieldMemberMetadataNode(..., name, fieldClass)construction matches the newer API and should compile cleanly.
- Verify whether
org.eclipse.tracecompass.ctf.core.trace.CTFTraceneeds explicit cleanup (e.g.,dispose()/close()) even when instantiated empty in tests.- Ensure the test bundle declares Gson as an OSGi import (if not already), since this class directly imports
com.google.gson.*.Also applies to: 62-80
🧹 Nitpick comments (2)
ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/FloatDeclarationParserTest.java (1)
24-26: PreferJsonObject#addProperty(...)over manualJsonPrimitivewrapping
This is more idiomatic Gson and reduces noise in the tests.-import com.google.gson.JsonPrimitive; @@ - fieldClass.add("length", new JsonPrimitive(32)); - fieldClass.add("byte-order", new JsonPrimitive("le")); + fieldClass.addProperty("length", 32); + fieldClass.addProperty("byte-order", "le"); @@ - fieldClass.add("length", new JsonPrimitive(64)); - fieldClass.add("byte-order", new JsonPrimitive("be")); - fieldClass.add("alignment", new JsonPrimitive(8)); + fieldClass.addProperty("length", 64); + fieldClass.addProperty("byte-order", "be"); + fieldClass.addProperty("alignment", 8);Also applies to: 41-44, 65-69
ctf/org.eclipse.tracecompass.ctf.core.tests/META-INF/MANIFEST.MF (1)
27-27: LGTM! Gson import added for JSON metadata parsing tests.The import is correctly placed and necessary for the JSON/TSdl metadata parsing capabilities introduced in this PR.
Optional improvement: Consider adding a version range to the Gson import for better dependency management, e.g.,
com.google.gson;version="[2.8.0,3.0.0)". This helps ensure compatibility and follows OSGi best practices, though the current approach is consistent with some other imports in this manifest (likecom.google.common.collect).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
ctf/org.eclipse.tracecompass.ctf.core.tests/META-INF/MANIFEST.MF(1 hunks)ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/CTF2IntegrationTest.java(1 hunks)ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/FloatDeclarationParserTest.java(1 hunks)ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/JsonTraceMetadataNodeTest.java(1 hunks)ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/TypeAliasParserTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/CTF2IntegrationTest.java
- ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/TypeAliasParserTest.java
🧰 Additional context used
🧬 Code graph analysis (1)
ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/JsonTraceMetadataNodeTest.java (1)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/JsonTraceMetadataNode.java (1)
JsonTraceMetadataNode(28-87)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
🔇 Additional comments (1)
ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/JsonTraceMetadataNodeTest.java (1)
33-50: Nice: focused assertions ongetEnvironment()andgetUid()outputs.The assertions are clear and validate the getters after
initialize().Also applies to: 60-69
| @Test | ||
| public void testEnvironmentParsing() throws Exception { | ||
| JsonTraceMetadataNode node = new JsonTraceMetadataNode(null, "trace-class", "test"); | ||
|
|
||
| JsonObject environment = new JsonObject(); | ||
| environment.addProperty("hostname", "test-host"); | ||
| environment.addProperty("domain", "kernel"); | ||
| environment.addProperty("tracer_name", "lttng-modules"); | ||
|
|
||
| // Simulate Gson deserialization by setting the field directly | ||
| java.lang.reflect.Field envField = JsonTraceMetadataNode.class.getDeclaredField("fEnvironment"); | ||
| envField.setAccessible(true); | ||
| envField.set(node, environment); | ||
|
|
||
| node.initialize(); | ||
|
|
||
| assertNotNull(node.getEnvironment()); | ||
| assertEquals("test-host", node.getEnvironment().get("hostname").getAsString()); | ||
| assertEquals("kernel", node.getEnvironment().get("domain").getAsString()); | ||
| assertEquals("lttng-modules", node.getEnvironment().get("tracer_name").getAsString()); | ||
| } |
There was a problem hiding this comment.
Avoid brittle reflection on private field fEnvironment (and hard-coded name) if a test seam exists.
This test will silently become invalid on any refactor/rename of JsonTraceMetadataNode.fEnvironment, and may fail under stronger encapsulation (JPMS / tightened setAccessible). Prefer constructing the node via the real Gson deserialization path, or add a test-only seam (e.g., package-private setter/constructor) to inject JsonObject without reflection.
If reflection is unavoidable, at least centralize field access to a helper to keep failures actionable and reduce duplication.
🤖 Prompt for AI Agents
In
ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/JsonTraceMetadataNodeTest.java
around lines 31 to 51, the test uses brittle reflection to set the private field
fEnvironment; replace this with a stable seam: either (preferred) construct the
node via real Gson deserialization by building a JSON string containing the
"environment" object and calling gson.fromJson(...) to obtain a
JsonTraceMetadataNode instance, or add a package-private/testing constructor or
setter on JsonTraceMetadataNode that accepts a JsonObject environment and use
that from the test; if reflection truly cannot be removed, centralize the
reflective access into a single private helper in the test that looks up the
field by name once and throws a clear, actionable exception on failure so
refactors fail loudly.
| @Test | ||
| public void testUidAndPacketHeader() throws Exception { | ||
| JsonTraceMetadataNode node = new JsonTraceMetadataNode(null, "trace-class", "test"); | ||
|
|
||
| // Simulate Gson deserialization | ||
| java.lang.reflect.Field uidField = JsonTraceMetadataNode.class.getDeclaredField("fUid"); | ||
| uidField.setAccessible(true); | ||
| uidField.set(node, "test-uid-123"); | ||
|
|
||
| node.initialize(); | ||
|
|
||
| assertEquals("test-uid-123", node.getUid()); | ||
| } |
There was a problem hiding this comment.
Test name doesn’t match coverage: no packet header is set/asserted.
testUidAndPacketHeader only asserts getUid(). Either rename to testUidParsing() or actually inject fPacketHeader (with a minimal JsonStructureFieldMetadataNode) and assert initialize() wires it (e.g., getPacketHeader() not null and/or child linkage if accessible).
🤖 Prompt for AI Agents
In
ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/JsonTraceMetadataNodeTest.java
around lines 58 to 70, the test named testUidAndPacketHeader only sets and
asserts the UID but does not set or assert any packet header; either rename the
test to testUidParsing() or modify it to inject a minimal fPacketHeader (create
a JsonStructureFieldMetadataNode instance, set it via reflection like fUid was
set), call initialize(), and add assertions that getPacketHeader() returns
non-null (and optionally verify the child linkage or expected type) to ensure
initialize() wires the packet header correctly.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/floatingpoint/FloatDeclarationParser.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/floatingpoint/FloatDeclarationParser.java (6)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/CTFJsonMetadataNode.java (1)
CTFJsonMetadataNode(33-171)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/JsonStructureFieldMemberMetadataNode.java (1)
JsonStructureFieldMemberMetadataNode(31-115)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/AlignmentParser.java (1)
AlignmentParser(51-117)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/ByteOrderParser.java (1)
ByteOrderParser(53-129)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/UnaryIntegerParser.java (1)
UnaryIntegerParser(28-68)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/utils/JsonMetadataStrings.java (1)
JsonMetadataStrings(25-257)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
🔇 Additional comments (3)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/floatingpoint/FloatDeclarationParser.java (3)
25-36: Imports properly support JSON metadata parsing.The new imports for JSON-based metadata handling are appropriate and necessary for the dual-path (JSON and legacy AST) parsing approach.
133-145: LENGTH-to-exponent/mantissa mapping is correct.The mapping follows IEEE 754 standard for 32-bit (8 exponent, 24 mantissa) and 64-bit (11 exponent, 53 mantissa) floating-point numbers, with appropriate error handling for unsupported sizes.
155-192: Legacy parsing path properly preserved.The existing AST-based parsing logic is maintained in the else branch with proper validation and error handling, ensuring backward compatibility while adding JSON support.
| if (fieldclass.has(JsonMetadataStrings.ALIGNMENT)) { | ||
| alignment = fieldclass.get(JsonMetadataStrings.ALIGNMENT).getAsInt(); | ||
| } |
There was a problem hiding this comment.
Validate alignment value in JSON parsing path.
The JSON path reads the alignment value directly without validation, while the legacy path (line 184) uses AlignmentParser.INSTANCE.parse() which validates that alignment must be positive and a power of 2. Invalid alignment values from JSON metadata (e.g., negative numbers or non-power-of-2 values like 3) would bypass validation and could cause issues downstream.
Consider applying this fix to validate alignment consistently:
if (fieldclass.has(JsonMetadataStrings.ALIGNMENT)) {
- alignment = fieldclass.get(JsonMetadataStrings.ALIGNMENT).getAsInt();
+ long alignmentValue = fieldclass.get(JsonMetadataStrings.ALIGNMENT).getAsLong();
+ if (alignmentValue <= 0 || (alignmentValue & (alignmentValue - 1)) != 0) {
+ throw new ParseException("Invalid value for alignment: " + alignmentValue);
+ }
+ alignment = alignmentValue;
}Alternatively, to maintain consistency with the existing validation logic, consider extracting the alignment validation into a reusable method or ensuring AlignmentParser can handle raw numeric values from JSON.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/floatingpoint/FloatDeclarationParser.java
around lines 152 to 154, the JSON branch assigns alignment directly from the
JSON value without validation; change this so the alignment value is validated
the same way as the legacy path (positive and power-of-two) by routing the JSON
numeric value through the existing AlignmentParser (e.g., call
AlignmentParser.INSTANCE.parse(...) with the JSON value) or by extracting the
alignment validation into a shared helper and using it here; ensure the final
alignment variable is only set when validation succeeds and handle invalid
values the same way as the legacy path (throw or error reporting).
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/TypeAliasParser.java (1)
213-220: Redefinition guard is only applied to the non-JSON path; apply the same protection to JSON parsing too.
Right now, JSON-based aliases can still redefine an existing type (or rely onregisterTypebehavior), while the ANTLR path rejects duplicates. Consider moving thelookupTypecheck to just beforeregisterType(...)so both branches are protected consistently.- scope.registerType(aliasString, targetDeclaration); + if (scope.lookupType(aliasString) != null) { + throw new ParseException("Type has already been defined: " + aliasString); //$NON-NLS-1$ + } + scope.registerType(aliasString, targetDeclaration); return targetDeclaration;
♻️ Duplicate comments (1)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/TypeAliasParser.java (1)
144-148: Fix possible NPE/IllegalStateException when readingfieldClass.TYPE.
fieldClass.get(JsonMetadataStrings.TYPE)can benull, andgetAsString()can throw if the element isn’t a primitive/string. This matches the previously reported issue.if (type.isEmpty()) { - if (fieldClass.isJsonObject()) { - type = fieldClass.get(JsonMetadataStrings.TYPE).getAsString(); - } + JsonElement typeElement = fieldClass.get(JsonMetadataStrings.TYPE); + if (typeElement != null && typeElement.isJsonPrimitive()) { + type = typeElement.getAsString(); + } }
🧹 Nitpick comments (3)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/TypeAliasParser.java (1)
27-36: Good extension of JSON TSDL coverage; consider tightening validation/error messages for unsupportedtypevalues.
The new parser delegations for strings/arrays/struct/floats are a straightforward extension and keepTypeAliasParseras the central dispatcher. One small improvement: whentypeis present but not a JSON string (or is empty), fail with a targetedParseExceptionrather than letting downstream code trip over unexpected shapes.Also applies to: 173-187
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/struct/StructDeclarationParser.java (2)
29-30: Consider usingjava.util.Objects.equals()instead of Guava's deprecated method.The import uses Guava's
Objects.equal(), which has been deprecated in favor of the standard library'sjava.util.Objects.equals(). Consider switching to the JDK method for future maintainability.Apply this diff to use the standard library method:
-import com.google.common.base.Objects; +import java.util.Objects;Then update line 138:
- if (current != null && (ctf1 || !Objects.equal(decl, current))) { + if (current != null && (ctf1 || !Objects.equals(decl, current))) {
97-99: Consider a more descriptive variable name thanctf1.The boolean
ctf1distinguishes between CTFAntlr (strict duplicate checking) and JSON (lenient duplicate checking) parsing paths. A more descriptive name likeisStrictDuplicateCheck,isCTFAntlrPath, oruseStrictDuplicateDetectionwould improve readability.Example refactor:
- boolean ctf1 = false; + boolean useStrictDuplicateDetection = false; if (declaration instanceof CTFAntlrMetadataNode) { - ctf1= true; + useStrictDuplicateDetection = true;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/metadata/DeclarationScope.java(3 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/IOStructGen.java(3 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/TypeAliasParser.java(4 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/struct/StructDeclarationParser.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/IOStructGen.java
🧰 Additional context used
🧬 Code graph analysis (1)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/TypeAliasParser.java (7)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/dynamicarray/DynamicLengthArrayParser.java (1)
DynamicLengthArrayParser(33-115)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/dynamicstring/DynamicLengthStringParser.java (1)
DynamicLengthStringParser(46-102)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/floatingpoint/FloatDeclarationParser.java (1)
FloatDeclarationParser(66-206)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/integer/IntegerDeclarationParser.java (1)
IntegerDeclarationParser(81-271)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/staticarray/StaticLengthArrayParser.java (1)
StaticLengthArrayParser(33-112)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/staticstring/StaticLengthStringParser.java (1)
StaticLengthStringParser(46-102)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/struct/StructParser.java (1)
StructParser(95-285)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
🔇 Additional comments (5)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/metadata/DeclarationScope.java (3)
20-20: LGTM!The import is necessary for the
Objects.equals()calls added inregisterTypeandregisterIdentifiermethods.
175-182: LGTM!The changes correctly implement duplicate-registration logic:
- Anonymous types (null/empty names) are skipped appropriately
- Existing declarations are retrieved by name and compared using
Objects.equals- Identical declarations can be re-registered (idempotent), while conflicting ones trigger a
ParseExceptionThis is a sound defensive approach for handling type registration.
200-201: LGTM! Critical bug fix confirmed.The changes correctly address the bug flagged in the past review comment:
- Now uses
fIdentifiers.get(name)instead of the incorrectfIdentifiers.get(declaration)- The map lookup uses the proper key type (String), making duplicate detection effective
- Identical declarations can be re-registered, while conflicts are properly caught
The implementation now matches the pattern used in
registerTypeand works as intended.ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/struct/StructDeclarationParser.java (2)
143-145: The simplified null check assumes prior validation.The condition now only checks
fieldName != null, relying on the exception thrown at line 136 to ensuredeclis non-null. This is only safe after fixing the ordering issue flagged in lines 133-137. Once that critical issue is resolved, this simplification is appropriate.
138-141: The duplicate field detection logic is intentional but lacks documentation and test coverage.The code correctly allows CTF2 to idempotently redeclare fields with identical declarations (same name, same type), while CTF1 strictly rejects any duplicate field. This pattern is consistent with
DeclarationScope.registerIdentifier()andDeclarationScope.registerVariant(), which both use the sameObjects.equals()comparison to allow equal redeclarations.However, this significant behavioral difference should be documented with:
- A comment explaining why CTF2 allows idempotent field redeclarations while CTF1 does not
- A test case in
CTF2IntegrationTestverifying that duplicate fields with identical declarations are accepted in CTF2 metadataThis will clarify the intent and prevent future confusion about why the check uses
Objects.equal(decl, current)instead of always rejecting duplicates.
| scope.registerIdentifier(fieldName, decl); | ||
| if (struct.hasField(fieldName)) { | ||
| IDeclaration current = struct.getField(fieldName); | ||
| if (decl == null) { | ||
| throw new ParseException("struct: Cannot add null field " + fieldName); //$NON-NLS-1$ | ||
| } |
There was a problem hiding this comment.
Critical: Null checks must occur before calling registerIdentifier and getField.
The current order performs operations with potentially null values before validation:
- Line 133 calls
scope.registerIdentifier(fieldName, decl)with potentially nullfieldName(when bothgetName()andgetRole()return null on lines 127-130) or nulldecl - Line 134 calls
struct.getField(fieldName)with potentially nullfieldName - Lines 135-137 check if
declis null, but by then it may already be registered in the scope
This can corrupt the scope state or cause NPEs. Move all null validations before line 133.
Apply this diff to fix the ordering:
scope.registerIdentifier(fieldName, decl);
- IDeclaration current = struct.getField(fieldName);
if (decl == null) {
throw new ParseException("struct: Cannot add null field " + fieldName); //$NON-NLS-1$
}
+ if (fieldName == null) {
+ throw new ParseException("struct: field name cannot be null"); //$NON-NLS-1$
+ }
+ IDeclaration current = struct.getField(fieldName);
if (current != null && (ctf1 || !Objects.equal(decl, current))) {
throw new ParseException("struct: duplicate field " //$NON-NLS-1$
+ fieldName);
}
-
- if (fieldName != null) {
- struct.addField(fieldName, decl);
- }
+ struct.addField(fieldName, decl);Note: You may also need to move the scope.registerIdentifier call after the null checks, depending on whether registering a null value is acceptable in the scope implementation.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/struct/StructDeclarationParser.java
around lines 133 to 137, the code calls scope.registerIdentifier(fieldName,
decl) and struct.getField(fieldName) before validating fieldName and decl; move
the null checks for fieldName (result of getName()/getRole()) and decl to before
any use of fieldName or decl, throw the ParseException if decl is null, and only
call scope.registerIdentifier(...) and struct.getField(...) after those
validations succeed (i.e., do not register or query with a null name or
declaration). Ensure registerIdentifier is moved after the null checks so the
scope is not corrupted by null entries.
There was a problem hiding this comment.
If fieldName is null the parser will just skip adding the item.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check DeclarationScope.registerIdentifier implementation to see if it handles null fieldName/decl
ast-grep --pattern $'class DeclarationScope {
$$$
registerIdentifier($$$) {
$$$
}
$$$
}'
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check StructDeclaration.getField implementation to see if it handles null fieldName
ast-grep --pattern $'class StructDeclaration {
$$$
getField($$$) {
$$$
}
$$$
}'
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Also check if there are any null safety annotations on these methods
rg -n "registerIdentifier.*@Nullable|@NonNull.*registerIdentifier" --type java
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check getField method signatures
rg -n "getField.*@Nullable|@NonNull.*getField" --type java -A2 -B2
Length of output: 6860
🏁 Script executed:
#!/bin/bash
# Find DeclarationScope class file
fd -t f "DeclarationScope.java" --exec cat {}
Length of output: 20583
🏁 Script executed:
#!/bin/bash
# Find StructDeclaration.getField method implementation
rg -n "public.*getField\(" ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/StructDeclaration.java -A10
Length of output: 318
🏁 Script executed:
#!/bin/bash
# Also check if there's a hasField method in StructDeclaration
rg -n "hasField" ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/StructDeclaration.java -B2 -A5
Length of output: 623
@MatthewKhouzam, you're absolutely right that the struct itself won't be corrupted—the guard at line 143 (if (fieldName != null)) ensures addField is only called with a valid field name.
However, there's still a concern at line 133: scope.registerIdentifier(fieldName, decl) is called before any null checks. Looking at the implementation, while this won't throw an NPE (HashMap accepts null keys), it will add a null entry to the scope's identifier registry if fieldName is null, which could pollute the scope state.
Similarly, line 134 calls struct.getField(fieldName) with a potentially null fieldName, though this just returns null safely.
If the intention is to skip processing entirely when fieldName is null (as you've indicated), would it make sense to add an early return or null check before line 133 to avoid these unnecessary operations and keep the scope clean? Something like:
if (fieldName == null) {
return struct; // or log a warning if appropriate
}
scope.registerIdentifier(fieldName, decl);
// ... rest of the logicBut if having a null entry in the identifier map is acceptable/expected behavior in your design, then the current flow is fine for that use case.
🧬 Code Graph Analysis Results
StructDeclarationParser.java
-
Snippet 1 (excerpt showing conditional addition when fieldName is non-null)
File: ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/struct/StructDeclarationParser.java
Content:scope.registerIdentifier(fieldName, decl); IDeclaration current = struct.getField(fieldName); if (decl == null) { throw new ParseException("struct: Cannot add null field " + fieldName); //$NON-NLS-1$ } if (current != null && (ctf1 || !Objects.equal(decl, current))) { throw new ParseException("struct: duplicate field " //$NON-NLS-1$ + fieldName); } if (fieldName != null) { struct.addField(fieldName, decl); }Note: This area demonstrates that if fieldName is null, the addField call is skipped, aligning with the user comment about skipping when fieldName is null.
-
Snippet 2 (context where fieldName is assigned for the CTF1 path)
File: ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/struct/StructDeclarationParser.java
Content:for (ICTFMetadataNode typeDeclaratorNode : typeDeclaratorList) { decl = TypeDeclaratorParser.INSTANCE.parse(typeDeclaratorNode, new TypeDeclaratorParser.Param(trace, typeSpecifierListNode, scope, identifierSB)); } fieldName = identifierSB.toString();Note: This shows the path where fieldName is derived from the parsed identifier. If this results in null, the subsequent conditional addition would skip adding the field, consistent with the observed behavior described in the user comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
common/org.eclipse.tracecompass.common.core.tests/META-INF/MANIFEST.MF (1)
24-24: LGTM! Consider adding a version range for the Gson import.The addition of
com.google.gsoncorrectly supports the new JSON metadata parsing functionality introduced in this PR. However, OSGi best practice recommends specifying version ranges for imported packages to ensure compatibility and prevent runtime issues.Consider applying this diff to add a version range:
- com.google.gson, + com.google.gson;version="[2.8.0,3.0.0)",Adjust the version range to match the actual Gson version used in your project dependencies.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
common/org.eclipse.tracecompass.common.core.tests/META-INF/MANIFEST.MF(1 hunks)common/org.eclipse.tracecompass.common.core.tests/src/org/eclipse/tracecompass/common/core/tests/log/LoggerTest.java(23 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
🔇 Additional comments (2)
common/org.eclipse.tracecompass.common.core.tests/src/org/eclipse/tracecompass/common/core/tests/log/LoggerTest.java (2)
129-597: Solid refactoring that improves test maintainability.The consistent replacement of hardcoded string assertions with structured JSON validation is a good improvement. This approach:
- Makes tests more resilient to formatting changes
- Validates the structure of log messages systematically
- Centralizes the validation logic for easier maintenance
34-35: Verify Gson version compatibility forparseString()usage.The code at line 108 uses
JsonParser.parseString(), which requires Gson 2.8.6 or later. The target platform defines Gson as a dependency, but the exact version cannot be determined from the repository artifacts. Confirm the project uses a compatible Gson version.
...compass.common.core.tests/src/org/eclipse/tracecompass/common/core/tests/log/LoggerTest.java
Show resolved
Hide resolved
MatthewKhouzam
force-pushed
the
ctf-2-fix-draft
branch
from
43b5e24 to
36c3178
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/BlobDeclaration.java (1)
65-68: Guard condition is always false for non-negative lengths.The condition
fLength > getMaximumSize()at line 66 checks iffLength > fLength * 8, which is always false for non-negativefLengthvalues (and could only be true due to integer overflow iffLength * 8wraps negative).If the intent was to remove the previous 1MB limit, consider removing the dead guard entirely. If some limit is still desired, the logic needs correction.
@Override public @NonNull BlobDefinition createDefinition(IDefinitionScope definitionScope, @NonNull String fieldName, @NonNull BitBuffer input) throws CTFException { - if (fLength > getMaximumSize()) { - throw new CTFException("Length asked: " + fLength + " is larger than the maximum blob size " + getMaximumSize()); //$NON-NLS-1$ //$NON-NLS-2$ - } byte[] array = new byte[fLength];Also applies to: 90-92
♻️ Duplicate comments (9)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/staticarray/StaticLengthArrayParser.java (1)
65-65: Length validation still missing.The past review comment correctly identified that the length should be validated before use to ensure it's positive and within reasonable bounds. This validation has not yet been implemented.
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/types/StaticLengthStringDeclaration.java (3)
23-28: Fix class Javadoc to describe static-length string.The Javadoc still says "Dynamic-length string declaration", but this class is
StaticLengthStringDeclarationfor fixed-length strings. Please update the summary line accordingly while keeping the encoding mention and author.Suggested change:
-/** - * Dynamic-length string declaration with encoding support +/** + * Static-length string declaration with encoding support * * @author Matthew Khouzam */
33-44: Align constructor Javadoc with actual parameters.The Javadoc documents a
lengthNameparameter that does not exist; the constructor takesint length. This can mislead readers and tools relying on Javadoc.Suggested change:
/** * Constructor * - * @param lengthName - * the name of the length field + * @param length + * the fixed length of the string in bytes * @param encoding * the character encoding */
96-99: CorrecttoString()label to “static_string”.
toString()currently returns"dynamic_string[...]", which contradicts the static-length nature of this declaration.Suggested change:
@Override public String toString() { - return "dynamic_string[" + fLength + "]<" + fEncoding.name() + ">"; //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ + return "static_string[" + fLength + "]<" + fEncoding.name() + ">"; //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ }ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/struct/StructParser.java (1)
211-218: Missing null checks for JSON member properties (previously flagged).The code at lines 213-215 still lacks null checks before calling
getAsString()andgetAsJsonObject()on theNAMEandFIELD_CLASSelements, risking NullPointerException for malformed input.ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/ScopedDefinition.java (1)
165-170: Infinite recursion bug (previously flagged).Line 169 calls
lookupDefinition(lookupPath, defintionToExclude)which is the same method, causing infinite recursion andStackOverflowError. This should delegate to the single-argument version:- return lookupDefinition(lookupPath, defintionToExclude); - }} + return lookupDefinition(lookupPath); + } +}ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/VariantDeclaration.java (1)
49-49: Thread safety issue withfFieldArrayremains unaddressed.The past review correctly identified that
fFieldsis a synchronized map, butfFieldArrayis accessed and modified without synchronization. This can cause race conditions if multiple threads calladdFieldconcurrently.Additionally, there's a minor formatting issue on line 173.
public void addField(String fieldTag, IDeclaration declaration) { + synchronized (fFields) { fFields.put(fieldTag, declaration); int size = fFields.size(); fFieldArray = Arrays.copyOf(fFieldArray, size); - fFieldArray[size-1]=declaration; + fFieldArray[size - 1] = declaration; + } }Also applies to: 169-174
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/floatingpoint/FloatDeclarationParser.java (1)
152-154: Alignment validation missing in JSON parsing path.The past review correctly identified that alignment is read directly without validation in the JSON path, while the legacy path (line 184) uses
AlignmentParser.INSTANCE.parse()which validates that alignment is positive and a power of 2. This issue remains unaddressed.Apply this fix to validate alignment consistently:
if (fieldclass.has(JsonMetadataStrings.ALIGNMENT)) { - alignment = fieldclass.get(JsonMetadataStrings.ALIGNMENT).getAsInt(); + long alignmentValue = fieldclass.get(JsonMetadataStrings.ALIGNMENT).getAsLong(); + if (alignmentValue <= 0 || (alignmentValue & (alignmentValue - 1)) != 0) { + throw new ParseException("Invalid value for alignment: " + alignmentValue); + } + alignment = alignmentValue; }ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/types/DynamicLengthStringDeclaration.java (1)
32-44: Enforce non‑null contract forlengthName/fLengthNameto avoid NPEs
lengthNameis annotated@Nullable, butfLengthNameis used everywhere as if non‑null (lookup, error message,toString) with no null checks. This creates a mismatch between the declared contract and actual usage and can lead to NPEs if a null ever gets passed.Since the parser always provides a valid name, it’s safer to enforce non‑null at the API boundary and on the field.
Suggested diff:
@@ -import org.eclipse.jdt.annotation.Nullable; +import org.eclipse.jdt.annotation.NonNull; +import org.eclipse.jdt.annotation.Nullable; @@ - private final Charset fEncoding; - private final String fLengthName; + private final Charset fEncoding; + private final @NonNull String fLengthName; @@ - * @param lengthName the name of the length field + * @param lengthName the name of the length field @@ - public DynamicLengthStringDeclaration(@Nullable String lengthName, Charset encoding) { + public DynamicLengthStringDeclaration(@NonNull String lengthName, Charset encoding) { fLengthName = lengthName; fEncoding = encoding; }After this change, please recompile with null‑analysis enabled to confirm there are no annotation conflicts with callers.
🧹 Nitpick comments (11)
common/org.eclipse.tracecompass.common.core.tests/META-INF/MANIFEST.MF (1)
24-24: LGTM. Optional: Consider adding a version range to match project practice.The Gson import is correct and properly formatted. However, there is precedent in the project for specifying versions on Gson imports:
doc/org.eclipse.tracecompass.examples.core/META-INF/MANIFEST.MFusescom.google.gson;version="2.8.2". Since this project does specify versions on other imports (e.g.,org.junit;bundle-version="4.0.0"), adding a version constraint would improve consistency.ctf/org.eclipse.tracecompass.tmf.ctf.core/src/org/eclipse/tracecompass/tmf/ctf/core/event/CtfTmfEventField.java (1)
121-126:StringDefinition2handling is consistent; please confirm type hierarchy / orderingMapping
StringDefinition2toCTFStringFieldviagetValue()mirrors the existingStringDefinitionhandling and looks correct.One thing to double‑check: if
StringDefinition2were ever to extendStringDefinition, theinstanceof StringDefinitionbranch would catch it first and thisStringDefinition2branch would be unreachable. Today this is probably fine (and suggestsStringDefinition2is a separate type), but it may be worth confirming that there is no inheritance relationship, or documenting that assumption here to avoid future surprises if the hierarchy evolves.ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/staticarray/StaticLengthArrayParser.java (1)
69-69: Consider adding a comment explaining the empty string parameters.The element node is constructed with empty strings for
type,value, andnameparameters. While this appears intentional (since the actual metadata is derived fromelementFieldClass), a brief comment explaining why these are empty would improve code clarity.ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/types/StaticLengthStringDeclaration.java (1)
64-94: Use an integral max-length literal and double‑check maximum size semantics.Two small points here:
1e6is a double literal; using an integral literal avoids implicit widening/narrowing and better reflects the domain (byte length):
if (rawLength > 1e6) {
if (rawLength > 1_000_000L) {
getMaximumSize()returnsInteger.MAX_VALUEeven though this declaration has a fixed length. Consider whether this should instead reflectfLength(in whatever unitsgetMaximumSizeis defined for), or at least be consistent with how other string declarations implement it in this codebase.ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/IntegerDefinition.java (1)
44-44: Lazy initialization introduces a benign data race.The
fMappingfield is now non-final and lazily initialized ingetMappings()without synchronization. While this could result in redundant computation under concurrent access, the computation is idempotent (same result each time), so correctness is preserved. If this code is accessed from multiple threads, consider using a local variable pattern to avoid potential visibility issues:public String getMappings() { String mapping = fMapping; if (mapping == null) { mapping = getDeclaration().getMappingForValue(fValue); fMapping = mapping; } return mapping; }Also applies to: 110-113
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/VariantDefinition.java (1)
166-192: Consider reducing code duplication between lookup methods.The two
lookupDefinitionoverloads share nearly identical logic. The first method could delegate to the second to reduce duplication:@Override public IDefinition lookupDefinition(String lookupPath) { - if (lookupPath == null) { - return null; - } - if (lookupPath.equals(fFieldName)) { - return fDefinition; - } - if (lookupPath.equals(fTagDef.getDeclaration().getRole())) { - return fTagDef; - } - if (lookupPath.equals(fDefinition.getDeclaration().getRole())) { - return fDefinition; - } - if (fDefinition instanceof ScopedDefinition) { - IDefinition def = ((ScopedDefinition) fDefinition).lookupDefinition(lookupPath, this); - if (def != null) { - return def; - } - } - final IDefinitionScope definitionScope = getDefinitionScope(); - if (definitionScope instanceof StructDefinition) { - StructDefinition structDefinition = (StructDefinition) definitionScope; - return structDefinition.lookupDefinition(lookupPath, this); - } - return definitionScope.lookupDefinition(lookupPath); + return lookupDefinition(lookupPath, this); }ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/StructDefinition.java (1)
187-187: Typo in parameter name: "defintionToExclude"The parameter is misspelled as
defintionToExclude(missing 'i' in "definition"). While this doesn't affect functionality, it impacts code readability and should be corrected throughout the file for consistency.Apply this diff to fix the typo:
- public Definition lookupDefinition(String lookupPath, ScopedDefinition defintionToExclude) { - if (this.equals(defintionToExclude) || lookupPath == null) { + public Definition lookupDefinition(String lookupPath, ScopedDefinition definitionToExclude) { + if (this.equals(definitionToExclude) || lookupPath == null) { return null; } /* * The fields are created in order of appearance, so if a variant or * sequence refers to a field that is after it, the field's definition * will not be there yet in the hashmap. */ int val = fFieldNames.indexOf(lookupPath); if (val != -1) { return fDefinitions[val]; } String lookupUnderscored = "_" + lookupPath; //$NON-NLS-1$ val = fFieldNames.indexOf(lookupUnderscored); if (val != -1) { return fDefinitions[val]; } for (IDefinition child : fDefinitions) { if (child instanceof ScopedDefinition) { - if (!child.equals(defintionToExclude)) { + if (!child.equals(definitionToExclude)) { if (lookupPath.equals(child.getDeclaration().getRole())) { if (child instanceof Definition) { return (Definition) child; } } - IDefinition def = ((ScopedDefinition) child).lookupDefinition(lookupPath, defintionToExclude); + IDefinition def = ((ScopedDefinition) child).lookupDefinition(lookupPath, definitionToExclude); if (def instanceof Definition) { return (Definition) def; } } } }ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/StructDeclaration.java (1)
422-430: Fix Javadoc for getRole methodThe
@returndocumentation states "the role" but the method actually returns theIDeclarationassociated with that role.Apply this diff to correct the Javadoc:
/** * Get the child with a role * @param role the role - * @return the role + * @return the declaration with the specified role, or null if not found * @since 5.1 */ public @Nullable IDeclaration getRole(String role) { return fRoles.get(role); }ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/TypeAliasParserTest.java (1)
66-81: Consider adding more specific assertionsThe test only verifies that the result is non-null. Consider adding assertions to validate specific properties of the parsed string declaration (e.g., length, encoding) to make the test more robust.
ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/FloatDeclarationParserTest.java (2)
38-54: Past constructor issues are resolvedThe previous review comments flagged constructor signature mismatches, but the current code correctly uses the 5-parameter
JsonStructureFieldMemberMetadataNodeconstructor. The test logic and assertions are correct for IEEE 754 single-precision floats.Optionally, you could simplify the JsonObject construction by using
addPropertyinstead of wrapping primitives:JsonObject fieldClass = new JsonObject(); - fieldClass.add("length", new JsonPrimitive(32)); - fieldClass.add("byte-order", new JsonPrimitive("le")); + fieldClass.addProperty("length", 32); + fieldClass.addProperty("byte-order", "le");
62-80: LGTM: Correct 64-bit float test with proper constructor usageThe test correctly validates IEEE 754 double-precision parsing with the proper 5-parameter constructor. Assertions match the expected exponent (11), mantissa (53), byte order, and alignment values.
Same optional simplification applies here:
JsonObject fieldClass = new JsonObject(); - fieldClass.add("length", new JsonPrimitive(64)); - fieldClass.add("byte-order", new JsonPrimitive("be")); - fieldClass.add("alignment", new JsonPrimitive(8)); + fieldClass.addProperty("length", 64); + fieldClass.addProperty("byte-order", "be"); + fieldClass.addProperty("alignment", 8);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
common/org.eclipse.tracecompass.common.core.tests/META-INF/MANIFEST.MF(1 hunks)common/org.eclipse.tracecompass.common.core.tests/src/org/eclipse/tracecompass/common/core/tests/log/LoggerTest.java(23 hunks)ctf/org.eclipse.tracecompass.ctf.core.tests/META-INF/MANIFEST.MF(1 hunks)ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/CTF2IntegrationTest.java(1 hunks)ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/FloatDeclarationParserTest.java(1 hunks)ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/JsonTraceMetadataNodeTest.java(1 hunks)ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/TypeAliasParserTest.java(1 hunks)ctf/org.eclipse.tracecompass.ctf.core/META-INF/MANIFEST.MF(1 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/metadata/DeclarationScope.java(3 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/BlobDeclaration.java(1 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/IntegerDeclaration.java(9 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/IntegerDefinition.java(2 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/ScopedDefinition.java(1 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/StringDefinition2.java(1 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/StructDeclaration.java(5 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/StructDefinition.java(4 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/VariantDeclaration.java(4 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/VariantDefinition.java(3 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/trace/Metadata.java(4 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/IOStructGen.java(3 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/JsonTraceMetadataNode.java(3 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/TypeAliasParser.java(4 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/dynamicarray/DynamicLengthArrayParser.java(1 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/dynamicstring/DynamicLengthStringParser.java(1 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/floatingpoint/FloatDeclarationParser.java(2 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/integer/IntegerDeclarationParser.java(1 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/staticarray/StaticLengthArrayParser.java(1 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/staticstring/StaticLengthStringParser.java(1 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/struct/StructDeclarationParser.java(4 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/struct/StructParser.java(2 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/variant/VariantParser.java(3 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/types/DynamicLengthStringDeclaration.java(1 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/types/StaticLengthStringDeclaration.java(1 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/utils/JsonMetadataStrings.java(5 hunks)ctf/org.eclipse.tracecompass.tmf.ctf.core/src/org/eclipse/tracecompass/tmf/ctf/core/event/CtfTmfEventField.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/variant/VariantParser.java
- ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/TypeAliasParser.java
- ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/JsonTraceMetadataNodeTest.java
- ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/trace/Metadata.java
- ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/struct/StructDeclarationParser.java
- ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/dynamicstring/DynamicLengthStringParser.java
- ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/staticstring/StaticLengthStringParser.java
- ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/dynamicarray/DynamicLengthArrayParser.java
- ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/StringDefinition2.java
- ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/utils/JsonMetadataStrings.java
- common/org.eclipse.tracecompass.common.core.tests/src/org/eclipse/tracecompass/common/core/tests/log/LoggerTest.java
- ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/CTF2IntegrationTest.java
- ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/metadata/DeclarationScope.java
🧰 Additional context used
🧬 Code graph analysis (6)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/struct/StructParser.java (4)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/JsonStructureFieldMemberMetadataNode.java (1)
JsonStructureFieldMemberMetadataNode(31-115)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/JsonStructureFieldMetadataNode.java (1)
JsonStructureFieldMetadataNode(29-89)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/AlignmentParser.java (1)
AlignmentParser(51-117)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/utils/JsonMetadataStrings.java (1)
JsonMetadataStrings(25-257)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/staticarray/StaticLengthArrayParser.java (3)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/TypeAliasParser.java (2)
NonNullByDefault(66-83)TypeAliasParser(55-222)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/types/ArrayDeclaration.java (1)
ArrayDeclaration(43-194)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/utils/JsonMetadataStrings.java (1)
JsonMetadataStrings(25-257)
ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/FloatDeclarationParserTest.java (2)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/JsonStructureFieldMemberMetadataNode.java (1)
JsonStructureFieldMemberMetadataNode(31-115)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/floatingpoint/FloatDeclarationParser.java (1)
FloatDeclarationParser(66-206)
ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/TypeAliasParserTest.java (2)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/JsonStructureFieldMemberMetadataNode.java (1)
JsonStructureFieldMemberMetadataNode(31-115)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/TypeAliasParser.java (1)
TypeAliasParser(55-222)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/types/StaticLengthStringDeclaration.java (3)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/io/BitBuffer.java (1)
BitBuffer(32-743)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/Declaration.java (1)
Declaration(30-90)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/StringDefinition2.java (1)
StringDefinition2(22-72)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/floatingpoint/FloatDeclarationParser.java (5)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/CTFJsonMetadataNode.java (1)
CTFJsonMetadataNode(33-171)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/JsonStructureFieldMemberMetadataNode.java (1)
JsonStructureFieldMemberMetadataNode(31-115)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/AlignmentParser.java (1)
AlignmentParser(51-117)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/UnaryIntegerParser.java (1)
UnaryIntegerParser(28-68)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/utils/JsonMetadataStrings.java (1)
JsonMetadataStrings(25-257)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
🔇 Additional comments (19)
ctf/org.eclipse.tracecompass.ctf.core.tests/META-INF/MANIFEST.MF (1)
27-27: LGTM! Gson import aligns with JSON-based test additions.The addition of
com.google.gsonto the Import-Package section is consistent with the broader JSON/TSdl parsing changes and new test classes mentioned in the AI summary (e.g., JsonTraceMetadataNodeTest). The lack of version constraint matches the pattern used in the core bundle and other imports in this manifest.ctf/org.eclipse.tracecompass.ctf.core/META-INF/MANIFEST.MF (1)
5-5: LGTM! Minor version bump is appropriate for CTF 2 support.The version bump from 5.0.2 to 5.1.0 correctly reflects the addition of new functionality (LTTng 2.14.3 CTF 2 trace support) while maintaining backward compatibility. This aligns with OSGi semantic versioning conventions for feature additions without breaking API changes.
ctf/org.eclipse.tracecompass.tmf.ctf.core/src/org/eclipse/tracecompass/tmf/ctf/core/event/CtfTmfEventField.java (1)
41-43: Import ofStringDefinition2is correct and usedThe new import cleanly integrates with the existing type imports and is used below in
parseField, so there’s no unused‑import or packaging concern here.ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/IntegerDeclaration.java (2)
635-660: LGTM on the mapping lookup fix.The backward iteration now correctly checks all candidates without early termination, ensuring intervals like
[0,100]are not missed when searching for values like 75. The logic is correct.Note: This approach is O(n) in the worst case when all intervals could potentially contain the value. For large mapping sets, consider an augmented interval tree, but this is likely acceptable for typical CTF use cases.
372-387: Consider thread safety for setMappings.The
setMappingsmethod modifiesfMappingsandfIntervalTreenon-atomically. If this method is called after construction whilegetMappingForValueis being invoked from another thread, it could result in inconsistent reads.If
setMappingsis guaranteed to be called only during construction (before the declaration is shared), this is fine. Otherwise, consider making the interval tree reference volatile or using copy-on-write semantics.ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/JsonTraceMetadataNode.java (1)
19-19: LGTM! Consistent library migration from org.json to Gson.The type change from
JSONObjecttoJsonObjectaligns with the Gson-based deserialization used throughout this class (via@SerializedNameannotations).Also applies to: 33-33, 65-65
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/VariantDeclaration.java (1)
127-158: Two-branch tag resolution logic is well-implemented.The implementation correctly handles:
- IntegerDefinition with mappings or numeric index-based lookup
- EnumDefinition with the preserved existing string-based lookup
- Proper bounds checking at lines 136-138 prevents
ArrayIndexOutOfBoundsException- Clear error messages for undefined tags and unknown selectors
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/VariantDefinition.java (2)
39-39: LGTM! Good API versioning and backward compatibility.The type widening from
EnumDefinitiontoSimpleDatatypeDefinitionallows handling both integer-based and enum-based tag definitions. The new constructor is properly annotated with@since 5.1, and the existingEnumDefinitionconstructor is preserved for backward compatibility.Also applies to: 45-69, 89-96
138-164: Role-based lookup logic is well-structured.The extended
lookupDefinitionmethod properly:
- Resolves fields by role using
getDeclaration().getRole()- Delegates to nested
ScopedDefinitionwith exclusion to prevent infinite loops- Falls back through the scope hierarchy appropriately
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/integer/IntegerDeclarationParser.java (1)
264-269: LGTM! Bug fix for mapped integer declaration creation.The fix correctly uses
(int) sizeas the first parameter for the mapped declaration path, aligning it with the non-mapped path on line 268. Previously usingbaseas the size would have created incorrectly-sized integer declarations.ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/floatingpoint/FloatDeclarationParser.java (2)
133-145: LGTM! IEEE 754 size mapping is correct.The implementation correctly maps:
- 32-bit → exponent=8, mantissa=24 (IEEE 754 binary32/float)
- 64-bit → exponent=11, mantissa=53 (IEEE 754 binary64/double)
The error handling for unsupported sizes provides a clear message.
155-192: Legacy parsing path correctly preserved.The existing TSDL parsing logic is cleanly moved to the else branch, preserving all attribute parsing including proper alignment validation via
AlignmentParser.INSTANCE.parse().ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/StructDefinition.java (3)
208-212: LGTM: Role-based lookup optimizationThe role-based short-circuit correctly checks if the
lookupPathmatches a child's declared role and returns it directly, avoiding unnecessary deep traversal.
223-225: LGTM: Cycle prevention in scope delegationThe addition of the exclusion parameter when delegating to the parent scope correctly prevents infinite recursion by passing
thisas the definition to exclude.
239-266: LGTM: Recursive role lookup with proper null safetyThe enhanced
lookupRolemethod correctly adds null-safety and introduces recursive traversal through nested structures (StructDefinition and VariantDefinition). The recursive helper properly handles the variant's current field selection.ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/StructDeclaration.java (3)
59-60: LGTM: Role-to-declaration mappingThe addition of
fRolesmap provides efficient O(1) lookup for declarations by role. The field is appropriately marked as final and non-null.
348-352: LGTM: Self-registration for role-based navigationThe override correctly propagates the role to the superclass and registers the struct itself in
fRoles, enabling parent structures to locate this struct by its role.
216-219: Add duplicate role detection to prevent silent overwritesWhen adding fields with roles, the code silently overwrites any existing declaration with the same role. Unlike field names (which trigger a warning if duplicated), roles have no such check. Add validation to detect and handle duplicate roles consistently.
ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/TypeAliasParserTest.java (1)
38-58: The type string"fixed-length-floating-point-number"in the test is correct and matchesJsonMetadataStrings.FIXED_LENGTH_FLOATING_POINTexactly. No changes needed.Likely an incorrect or invalid review comment.
| public void testDynamicLengthStringParsing() throws Exception { | ||
| // Test that the parser doesn't throw an exception for dynamic length strings | ||
| // The actual parsing logic may not be fully implemented yet | ||
| CTFTrace trace = new CTFTrace(); | ||
| DeclarationScope scope = new DeclarationScope(null, "test"); | ||
|
|
||
| JsonObject fieldClass = new JsonObject(); | ||
| fieldClass.add("type", new JsonPrimitive("dynamic-length-string")); | ||
| fieldClass.add("encoding", new JsonPrimitive("utf-8")); | ||
|
|
||
| JsonStructureFieldMemberMetadataNode node = new JsonStructureFieldMemberMetadataNode(null, "dynamic-length-string", "test", "dyn_string_field", fieldClass); | ||
|
|
||
| try { | ||
| IDeclaration result = TypeAliasParser.INSTANCE.parse(node, | ||
| new TypeAliasParser.Param(trace, scope)); | ||
| // If we get here without exception, the basic parsing works | ||
| assertNotNull(result); | ||
| } catch (Exception e) { | ||
| // For now, just verify the parser recognizes the type | ||
| assertTrue("Parser should handle dynamic-length-string type", | ||
| e.getMessage() == null || !e.getMessage().contains("Invalid field class")); | ||
| } | ||
| } |
There was a problem hiding this comment.
Clarify implementation status of dynamic-length string parsing
The comment and try-catch structure suggest the parsing logic "may not be fully implemented yet." If this feature is incomplete, consider:
- Marking the test with
@Ignoreand a TODO comment with a tracking issue - Or document the expected behavior more clearly in the test
- Or implement the missing functionality before merging
Tests that allow both success and certain failures make it difficult to detect regressions.
Do you want me to search the codebase to verify if DynamicLengthStringParser is fully implemented?
🤖 Prompt for AI Agents
In
ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/TypeAliasParserTest.java
around lines 89-111, the test for dynamic-length-string currently swallows
failures in a try/catch which makes regressions undetectable; first check
whether DynamicLengthStringParser is implemented—if it is not, annotate the test
with @Ignore and add a one-line TODO with a tracking issue/PR number explaining
why it’s ignored; if the parser is implemented, remove the lenient try/catch,
assert the parsed result (assertNotNull) and/or assert specific expected
behavior, and fail the test on any exception so the test reliably detects
regressions.
| for (Entry<String, JsonElement> entry : environment.entrySet()) { | ||
| env.put(entry.getKey(), entry.getValue().toString()); | ||
| } |
There was a problem hiding this comment.
JsonElement.toString() includes JSON quotes for string values.
Using entry.getValue().toString() on a JsonElement produces the JSON representation, which includes quotes around strings. For example, a JSON string "hello" will become "\"hello\"" as a Java String.
Consider handling primitives explicitly:
for (Entry<String, JsonElement> entry : environment.entrySet()) {
- env.put(entry.getKey(), entry.getValue().toString());
+ JsonElement value = entry.getValue();
+ if (value.isJsonPrimitive()) {
+ env.put(entry.getKey(), value.getAsString());
+ } else {
+ env.put(entry.getKey(), value.toString());
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (Entry<String, JsonElement> entry : environment.entrySet()) { | |
| env.put(entry.getKey(), entry.getValue().toString()); | |
| } | |
| for (Entry<String, JsonElement> entry : environment.entrySet()) { | |
| JsonElement value = entry.getValue(); | |
| if (value.isJsonPrimitive()) { | |
| env.put(entry.getKey(), value.getAsString()); | |
| } else { | |
| env.put(entry.getKey(), value.toString()); | |
| } | |
| } |
🤖 Prompt for AI Agents
In
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/IOStructGen.java
around lines 165-167, the code uses entry.getValue().toString() which preserves
JSON quoting for string primitives; change the logic to detect JsonElement types
and extract values without JSON quotes for primitives (use isJsonPrimitive() and
for string primitives use getAsString(), for numbers/booleans use appropriate
getAs...()), handle nulls with isJsonNull(), and fall back to toString() only
for complex objects/arrays so stored env values are plain Java values rather
than JSON-quoted strings.
| JsonStructureFieldMemberMetadataNode member = (JsonStructureFieldMemberMetadataNode) node; | ||
| JsonObject fieldClass = member.getFieldClass().getAsJsonObject(); |
There was a problem hiding this comment.
Add null and type validation for field class extraction.
The code calls getFieldClass().getAsJsonObject() without validating that getFieldClass() returns a non-null value or that it's actually a JsonObject. This could result in a NullPointerException or ClassCastException at runtime.
Apply this diff to add validation:
JsonStructureFieldMemberMetadataNode member = (JsonStructureFieldMemberMetadataNode) node;
-JsonObject fieldClass = member.getFieldClass().getAsJsonObject();
+JsonElement fieldClassElement = member.getFieldClass();
+if (fieldClassElement == null || !fieldClassElement.isJsonObject()) {
+ throw new ParseException("Static-length array requires a valid field-class object");
+}
+JsonObject fieldClass = fieldClassElement.getAsJsonObject();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| JsonStructureFieldMemberMetadataNode member = (JsonStructureFieldMemberMetadataNode) node; | |
| JsonObject fieldClass = member.getFieldClass().getAsJsonObject(); | |
| JsonStructureFieldMemberMetadataNode member = (JsonStructureFieldMemberMetadataNode) node; | |
| JsonElement fieldClassElement = member.getFieldClass(); | |
| if (fieldClassElement == null || !fieldClassElement.isJsonObject()) { | |
| throw new ParseException("Static-length array requires a valid field-class object"); | |
| } | |
| JsonObject fieldClass = fieldClassElement.getAsJsonObject(); |
🤖 Prompt for AI Agents
In
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/staticarray/StaticLengthArrayParser.java
around lines 52-53, avoid calling getFieldClass().getAsJsonObject() directly:
first retrieve the JsonElement into a local variable, check it for null, and
verify it is a JsonObject via isJsonObject(); only then call getAsJsonObject();
if the element is null or not a JsonObject, handle the condition
deterministically (e.g. throw a descriptive IllegalArgumentException or
return/skip with a logged error) so you prevent
NullPointerException/ClassCastException and provide a clear failure path.
| @Override | ||
| public boolean isBinaryEquivalent(IDeclaration other) { | ||
| // TODO Auto-generated method stub | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Implement isBinaryEquivalent instead of returning a TODO stub.
Leaving isBinaryEquivalent as a stub that always returns false breaks semantic equivalence for identical static-length string declarations and may affect type compatibility checks elsewhere.
Suggested implementation:
@Override
public boolean isBinaryEquivalent(IDeclaration other) {
- // TODO Auto-generated method stub
- return false;
+ if (!(other instanceof StaticLengthStringDeclaration)) {
+ return false;
+ }
+ StaticLengthStringDeclaration o = (StaticLengthStringDeclaration) other;
+ return fLength == o.fLength && fEncoding.equals(o.fEncoding);
}Optionally, if Declaration.getRole() is part of binary equivalence semantics, you may also want to include a role comparison similar to other declaration types in this project.
🤖 Prompt for AI Agents
In
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/types/StaticLengthStringDeclaration.java
around lines 101-105, replace the TODO stub by implementing proper binary
equivalence: check that other is a StaticLengthStringDeclaration, cast it, and
compare the intrinsic fields that define binary layout (e.g., the fixed length
value and any encoding/charset or null-termination/termination policy fields
present on this class) and optionally compare Declaration.getRole() if other
declaration implementations use role in equivalence; return true only if all
those binary-layout-defining fields are equal (handle nulls safely) and false
otherwise.
Change-Id: Ied093d7edf0e7de76e0dc637699711cf0653dc6b Signed-off-by: Matthew Khouzam <matthew.khouzam@ericsson.com>
Change-Id: I62cc5e26645ea1d2bd2150af99d5aeb878061e1f Signed-off-by: Matthew Khouzam <matthew.khouzam@ericsson.com>
Change-Id: Ie8752173c78caf0d0f5cef3cb1a5d2aa58c1e8d8 Signed-off-by: Matthew Khouzam <matthew.khouzam@ericsson.com>
It's a mix of a sequence and a string. A dynamic-length string field class is an abstract string field class which describes dynamic-length string fields. A static-length string field is a sequence of zero or more contiguous encoded Unicode codepoints. All the encoded codepoints of a dynamic-length string field before the first “NULL” (U+0000) codepoint, if any, form the resulting string value. The first U+0000 codepoint, if any, and all the following bytes are considered padding (garbage data). Change-Id: I19bbef5eb2913d8252e469e477363da7eecacdcb Signed-off-by: Matthew Khouzam <matthew.khouzam@ericsson.com>
Change-Id: Iaf2558ab50b1f47604da28f4529bd094135a4ce5 Signed-off-by: Matthew Khouzam <matthew.khouzam@ericsson.com>
Change-Id: Ic10d79e4e72a043e9116d554f5dffacd34013fa6 Signed-off-by: Matthew Khouzam <matthew.khouzam@ericsson.com>
Change-Id: Ib3654d5468236a9e3ab1a2a2b0cb49dbfc76e251 Signed-off-by: Matthew Khouzam <matthew.khouzam@ericsson.com>
Change-Id: I724d84c0b1f5c300ee73c4f2c2b0b9fa3f4ca3d1 Signed-off-by: Matthew Khouzam <matthew.khouzam@ericsson.com>
Change-Id: Icb35ac1c931e398be30debfa45c539f8c9fcd245 Signed-off-by: Matthew Khouzam <matthew.khouzam@ericsson.com>
Change-Id: I30afb71bbcf7ff688590b95b7f48caf53b9d3ed4 Signed-off-by: Matthew Khouzam <matthew.khouzam@ericsson.com>
Change-Id: Ie35533f33a482aff519f9472b0a34b088be1506e Signed-off-by: Matthew Khouzam <matthew.khouzam@ericsson.com>
Change-Id: I8a1408772b8e1ca2e634fa892e7d8a3c05349877 Signed-off-by: Matthew Khouzam <matthew.khouzam@ericsson.com>
Needed for expanding aliases Change-Id: I97039e706002602347cb6d06fbcfecbbd084b55d Signed-off-by: Matthew Khouzam <matthew.khouzam@ericsson.com>
Add variant definition constructor for CTF2 Change-Id: I21f56a284c6fcee147d3714445ed30d81b706c1a Signed-off-by: Matthew Khouzam <matthew.khouzam@ericsson.com>
Change-Id: Ief637e1c6bc49aca4bed87b586224df9383c1a9e Signed-off-by: Matthew Khouzam <matthew.khouzam@ericsson.com>
Remove infinite recurstion if something is not found. (due to roles in ctf2) Change-Id: Ifa62ed5807a17e307c044e066399cf15fae2bc74 Signed-off-by: Matthew Khouzam <matthew.khouzam@ericsson.com>
O(log(n)) instead of O(n) This patch was made with the assistance of Claude Sonnet 4.5 Change-Id: Ic46ff28a84e45beb8462f642340db19d04fa40fc Signed-off-by: Matthew Khouzam <matthew.khouzam@ericsson.com>
Found with CodeRabbit. Change-Id: Ifa4be68a71fc98e6ca4604c290d50d6840746451 Signed-off-by: Matthew Khouzam <matthew.khouzam@ericsson.com>
Change-Id: I600c3d760717956097e41cb2db44de0cc58086ce Signed-off-by: Matthew Khouzam <matthew.khouzam@ericsson.com>
Change-Id: I60f3854afcdf4454847ca49bfdee0ca7d89f9601 Signed-off-by: Matthew Khouzam <matthew.khouzam@ericsson.com>
optimizations negatively affect ctf2. Roles do not work with it. Change-Id: I34812fe76934c62f00ae3de10cdfad074e996f7f Signed-off-by: Matthew Khouzam <matthew.khouzam@ericsson.com>
Makes CTF too deep structs still work Change-Id: Ic8a75dd8c798945779bbcf2a7de3fcab7195decb Signed-off-by: Matthew Khouzam <matthew.khouzam@ericsson.com>
Change-Id: If58b118283f9a5e3778e832edd94e358c23cded8 Signed-off-by: Matthew Khouzam <matthew.khouzam@ericsson.com>
Change-Id: I95537016f0fbc331cc89c88de995d535c8dc1915 Signed-off-by: Matthew Khouzam <matthew.khouzam@ericsson.com>
Change-Id: I40faac6cc2590480c061824beb952d6a19cb2f12 Signed-off-by: Matthew Khouzam <matthew.khouzam@ericsson.com>
Change-Id: I018e6cfa1f3e7aeed14953de76d6d9f34adf8e64 Signed-off-by: Matthew Khouzam <matthew.khouzam@ericsson.com>
made in part by claude sonnet 4.5. Many bad hallucinations needed to be manually fixed. Change-Id: Ibee4b0cf144f2afa922ae968fd17d6065e18b257 Signed-off-by: Matthew Khouzam <matthew.khouzam@ericsson.com>
Generated by Claude Sonnet 4.5, manually fixed by me. Change-Id: Ieec23f0b0f783775a166b530c9c5c75bc0190c53 Signed-off-by: Matthew Khouzam <matthew.khouzam@ericsson.com>
Update ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/floatingpoint/FloatDeclarationParser.java Done by coderabbit. Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Change-Id: Ieebad19af2e0fd716d2373d563f802934f7a93ae Signed-off-by: Matthew Khouzam <matthew.khouzam@ericsson.com>
Do not accept double typealiases with the same value. Change-Id: I2affb15f6faf83cd0292a7310febbad8ae657513 Signed-off-by: Matthew Khouzam <matthew.khouzam@ericsson.com>
Clean up the hashcode. Fix a corner case in getMappings Change-Id: Iec698c9761fde045a0b9945e65ebcda97fa99cb4 Signed-off-by: Matthew Khouzam <matthew.khouzam@ericsson.com>
With Java 25, flows can go to different TIDs. This allows the tests to pass. Change-Id: Ic26850e16ce813ba2c70b0af71b9caa6af0805f2 Signed-off-by: Matthew Khouzam <matthew.khouzam@ericsson.com>
MatthewKhouzam
force-pushed
the
ctf-2-fix-draft
branch
from
36c3178 to
c2cdf0c
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (26)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/trace/Metadata.java (2)
301-316: Previously flagged issues inexpandAliasesremain unaddressed.Per past review:
- ConcurrentModificationException risk (line 302): Iterating
obj.keySet()while callingobj.add()can fail depending on Gson's implementation. Iterate over a copy:new ArrayList<>(obj.keySet()).- String aliases in arrays not expanded (lines 309-312): Only
JsonObjectelements are recursed into; string alias references directly in arrays won't be resolved.
259-266:JsonParser.parseStringcan throwJsonSyntaxExceptionoutside the try-catch block.If
jsonBlocks[i]contains malformed JSON,JsonParser.parseString()at line 259 throwsJsonSyntaxException, but the try-catch at lines 269-273 only coversgson.fromJson(). This could cause an unhandled exception and abort trace parsing.for (int i = 1; i < jsonBlocks.length; i++) { - JsonElement element = JsonParser.parseString(jsonBlocks[i]); + JsonElement element; + try { + element = JsonParser.parseString(jsonBlocks[i]); + } catch (JsonSyntaxException e) { + throw new CTFException("Trace cannot be parsed as CTF2", e); //$NON-NLS-1$ + } if (element.isJsonObject()) {
Add null checks before accessing
NAMEandFIELD_CLASSproperties (lines 263-265).This was flagged in a previous review. Malformed
FIELD_ALIASfragments missing these properties will throw ongetAsString()orgetAsJsonObject().ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/StringDefinition2.java (1)
24-24: Potential null pointer exception ifvalueis null.The
valueparameter in both constructors lacks@NonNullannotation, andfStringis stored directly. If a null value is passed,size()andtoString()will throw NPE.Also applies to: 34-37, 48-51
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/floatingpoint/FloatDeclarationParser.java (1)
152-154: Alignment value not validated in JSON parsing path.The JSON path reads alignment directly without validation, while the legacy path (line 184) uses
AlignmentParser.INSTANCE.parse()which validates that alignment must be positive and a power of 2. Invalid alignment values from JSON metadata could bypass validation.ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/dynamicstring/DynamicLengthStringParser.java (1)
65-74: Missing null-safety for chained JSON access.This issue was previously flagged. Line 70 chains multiple calls without null checks. If
lengthFieldLocationis not a JSON object or doesn't containPATH, this will throw an unchecked exception rather than a descriptiveParseException.ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/types/StaticLengthStringDeclaration.java (4)
23-27: Fix incorrect class javadoc.The javadoc says "Dynamic-length string declaration" but this class is
StaticLengthStringDeclarationfor static-length strings.
33-44: Fix constructor javadoc parameter mismatch.The javadoc refers to parameter
lengthNameof type String, but the actual parameter islengthof type int.
96-99: Correct toString() to indicate static-length string.The toString() method returns "dynamic_string" but this class represents a static-length string.
101-105: ImplementisBinaryEquivalentinstead of returning a TODO stub.Leaving
isBinaryEquivalentas a stub that always returnsfalsebreaks semantic equivalence for identical static-length string declarations.ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/utils/JsonMetadataStrings.java (5)
184-187: Incorrect Javadoc for DYNAMIC_LENGTH_STRING constant.The Javadoc says "Type string for a null terminated string field class" but this constant represents
"dynamic-length-string".
203-206: Incorrect Javadoc for PATH constant.The Javadoc says "Type string for a structure field class" but this constant represents
"path".
220-222: UTF-32 charset handling should account for JDK version compatibility.
Charset.forName("UTF-32BE")andCharset.forName("UTF-32LE")will throwUnsupportedCharsetExceptionat class loading time on JDKs prior to version 22.
249-251: Missing Javadoc for public constants.
MEMBER_CLASSESandMINIMUM_ALIGNMENTare missing Javadoc comments.
253-255: Missing Javadoc and externalized string markers.
ALIGNMENTandBYTE_ORDERconstants are missing Javadoc comments and//$NON-NLS-1$markers.ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/variant/VariantParser.java (2)
269-271: Add defensive checks for JSON parsing to prevent unhelpful exceptions.Line 270 assumes
SELECTOR_FIELD_LOCATIONcontains a JSON object with aPATHfield that is an array. Thehas()check only verifies the key exists, not the structure.
365-374: Error message is misleading after adding IntegerDeclaration support.The code now accepts both
EnumDeclarationandIntegerDeclarationas valid tag types, but the error message at line 373 still says "Variant tag must be an enum".ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/IOStructGen.java (1)
165-167:JsonElement.toString()includes JSON quotes for string values.Using
entry.getValue().toString()on aJsonElementproduces the JSON representation, which includes quotes around strings. For example, a JSON string"hello"will become"\"hello\""as a Java String.ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/struct/StructParser.java (1)
211-217: Missing null checks for JSON member properties.
memberElement.getAsJsonObject().get(JsonMetadataStrings.NAME)andget(JsonMetadataStrings.FIELD_CLASS)can returnnull, causing NullPointerException whengetAsString()orgetAsJsonObject()is called.ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/JsonTraceMetadataNodeTest.java (2)
31-51: Reflection-based injection offEnvironmentis brittleThis test directly sets
JsonTraceMetadataNode.fEnvironmentvia reflection. That’s fragile against refactors (field rename/visibility changes) and may eventually break under stricter module access. Prefer going through the actual Gson deserialization path or a package-private/testing seam (e.g., constructor or setter) to inject the environment.If reflection must stay, centralize it in a helper so failures are easier to diagnose.
53-70:testUidAndPacketHeaderonly asserts UID, not packet headerDespite the name, this test only sets and asserts the UID; no packet-header setup or assertion is performed. Either rename the test to reflect what it actually covers, or extend it to initialize a minimal
fPacketHeaderand verify thatinitialize()wires it correctly (e.g.,getPacketHeader()non-null and/or child linkage).ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/TypeAliasParser.java (1)
144-187: Add null/type checks when inferringtypefromfieldClassHere:
if (type.isEmpty()) { if (fieldClass.isJsonObject()) { type = fieldClass.get(JsonMetadataStrings.TYPE).getAsString(); } }
fieldClass.get(JsonMetadataStrings.TYPE)can returnnullor a non‑primitive value, leading to aNullPointerExceptionorIllegalStateExceptionongetAsString(). This was already flagged in an earlier review.Consider guarding the access:
- if (type.isEmpty()) { - if (fieldClass.isJsonObject()) { - type = fieldClass.get(JsonMetadataStrings.TYPE).getAsString(); - } - } + if (type.isEmpty()) { + JsonElement typeElement = fieldClass.get(JsonMetadataStrings.TYPE); + if (typeElement != null && typeElement.isJsonPrimitive()) { + type = typeElement.getAsString(); + } + }The added branches for dynamic/static strings and arrays, structs, and floating point types, as well as the alias redefinition check, otherwise look consistent with the existing parser design.
Also applies to: 213-215
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/ScopedDefinition.java (1)
155-170: Fix infinite recursion inlookupDefinition(String, ScopedDefinition)The two-argument overload currently calls itself with identical parameters, causing infinite recursion and a
StackOverflowErrorat runtime.Replace the self-call with a delegation to the single-argument overload so the base behavior is preserved and subclasses can override the exclusion-aware form:
- public @Nullable IDefinition lookupDefinition(@Nullable String lookupPath, @Nullable ScopedDefinition defintionToExclude) { - if(defintionToExclude == this) { - return null; - } - return lookupDefinition(lookupPath, defintionToExclude); - }} + public @Nullable IDefinition lookupDefinition(@Nullable String lookupPath, @Nullable ScopedDefinition defintionToExclude) { + if (defintionToExclude == this) { + return null; + } + return lookupDefinition(lookupPath); + } +}ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/staticarray/StaticLengthArrayParser.java (1)
43-73: Harden JSON handling forfield-classand validate array lengthTwo robustness issues here (both previously raised):
- Potential NPE/ClassCastException on
getFieldClass().getAsJsonObject()JsonStructureFieldMemberMetadataNode member = (JsonStructureFieldMemberMetadataNode) node; JsonObject fieldClass = member.getFieldClass().getAsJsonObject();If
getFieldClass()is null or not a JSON object, this will fail at runtime. Consider:- JsonStructureFieldMemberMetadataNode member = (JsonStructureFieldMemberMetadataNode) node; - JsonObject fieldClass = member.getFieldClass().getAsJsonObject(); + JsonStructureFieldMemberMetadataNode member = (JsonStructureFieldMemberMetadataNode) node; + JsonElement fieldClassElement = member.getFieldClass(); + if (fieldClassElement == null || !fieldClassElement.isJsonObject()) { + throw new ParseException("Static-length array requires a valid field-class object"); //$NON-NLS-1$ + } + JsonObject fieldClass = fieldClassElement.getAsJsonObject();
- No validation of
lengthField.getAsInt()
lengthFieldis only checked for presence; negative or unreasonably large values are accepted:int length = lengthField.getAsInt();At minimum, enforce non-negative (and optionally a sane upper bound) to avoid malformed metadata causing pathological allocations:
- int length = lengthField.getAsInt(); + int length = lengthField.getAsInt(); + if (length < 0) { + throw new ParseException("Static-length array length must be non-negative, got: " + length); //$NON-NLS-1$ + }(If you have an agreed maximum length, you can also enforce it here.)
The rest of the parsing logic (element field class resolution via
TypeAliasParserand construction ofArrayDeclaration) looks good.ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/staticstring/StaticLengthStringParser.java (3)
29-45: Javadoc still describes dynamic-length instead of static-length.This documentation issue was already flagged in a previous review but remains unaddressed.
57-68: Error messages still reference "Dynamic-length" instead of "Static-length".The error messages at lines 59 and 67 incorrectly reference dynamic-length strings and wrong property names. This was previously flagged but not yet fixed.
77-79: Param Javadoc still references wrong parser type.This documentation issue was previously flagged but remains unaddressed.
🧹 Nitpick comments (7)
ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/FloatDeclarationParserTest.java (1)
24-26: Optional: Simplify JSON construction usingaddProperty.The current code uses
addwithJsonPrimitivewrappers, which works correctly. However,JsonObject.addPropertyis more idiomatic and eliminates the need for explicitJsonPrimitiveinstantiation.Apply this diff to simplify:
-import com.google.gson.JsonPrimitive; JsonObject fieldClass = new JsonObject(); - fieldClass.add("length", new JsonPrimitive(32)); - fieldClass.add("byte-order", new JsonPrimitive("le")); + fieldClass.addProperty("length", 32); + fieldClass.addProperty("byte-order", "le"); @@ JsonObject fieldClass = new JsonObject(); - fieldClass.add("length", new JsonPrimitive(64)); - fieldClass.add("byte-order", new JsonPrimitive("be")); - fieldClass.add("alignment", new JsonPrimitive(8)); + fieldClass.addProperty("length", 64); + fieldClass.addProperty("byte-order", "be"); + fieldClass.addProperty("alignment", 8);Also applies to: 42-43, 66-68
ctf/org.eclipse.tracecompass.tmf.ctf.ui.swtbot.tests/src/org/eclipse/tracecompass/tmf/ctf/ui/swtbot/tests/TestInvalidCtfTrace.java (1)
205-211: Consider making the label search loop more robustThe loop assumes the relevant text will always be found within the first few labels and may attempt to read beyond the “3 labels” mentioned in the comment if the message is ever missing or the dialog layout changes. A more defensive pattern (e.g., iterating up to
dialogBot.labels().size()or breaking onceireaches the last index) would make the test less brittle to UI changes, while still validating the same behavior.ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/trace/Metadata.java (1)
286-288: Prefer checking the parsedJsonObjectinstead of raw string contains.The string check
jsonBlocks[i].contains("id:")is fragile—it could match"id:"appearing in string values or other contexts. Since the JSON is already parsed at line 261, consider checking the parsed object instead.} else if (type.equals(JsonMetadataStrings.FRAGMENT_DATA_STREAM)) { fragment = Objects.requireNonNull(gson.fromJson(element, JsonDataStreamMetadataNode.class)); - if (!jsonBlocks[i].contains("id:")) { //$NON-NLS-1$ + JsonObject dsObj = element.getAsJsonObject(); + if (!dsObj.has("id")) { //$NON-NLS-1$ ((JsonDataStreamMetadataNode) fragment).setId(-1); }ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/StringDefinition2.java (1)
34-52: Consider consolidating constructors or enforcing type safety.Both constructors perform identical logic, differing only in the declaration parameter type. The second constructor accepts any
Declaration, weakening type safety compared to the first which requiresStringDeclaration.If the intent is to support both
StringDeclarationandDynamicLengthStringDeclaration, consider using a common base type or interface that both implement, rather than accepting the genericDeclarationbase class.ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/dynamicstring/DynamicLengthStringParser.java (1)
77-101: UnusedfTracefield inParamclass.The
Paramclass stores aCTFTracereference that is never used by theparse()method. Either this is dead code or the implementation is incomplete.Consider either removing the unused field or documenting why it's reserved for future use:
@NonNullByDefault public static final class Param implements ICommonTreeParserParameter { - private final CTFTrace fTrace; - /** * Parameter constructor - * - * @param trace the trace */ - public Param(CTFTrace trace) { - fTrace = trace; + public Param() { + // No parameters needed currently } - - /** - * Get the trace - * - * @return the trace - */ - public CTFTrace getTrace() { - return fTrace; - } }Alternatively, if the trace is needed for future enhancements, add a comment explaining the intent.
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/types/StaticLengthStringDeclaration.java (1)
64-73: Redundant length validation and inconsistent type usage.Since
fLengthis already anint, the negative check on line 67 is only meaningful if the constructor receives a negative int. The assignment tolong rawLengthon line 66 is unnecessary indirection. Additionally, using1e6(a double) for comparison is unusual.@Override public StringDefinition2 createDefinition(@Nullable IDefinitionScope definitionScope, String fieldName, BitBuffer input) throws CTFException { - long rawLength = fLength; - if (rawLength < 0) { - throw new CTFException("Cannot have a length < 0, declared = " + rawLength); //$NON-NLS-1$ + if (fLength < 0) { + throw new CTFException("Cannot have a length < 0, declared = " + fLength); //$NON-NLS-1$ } - if (rawLength > 1e6) { - throw new CTFException("Cannot have a length > 1000000, declared = " + rawLength); //$NON-NLS-1$ + if (fLength > 1_000_000) { + throw new CTFException("Cannot have a length > 1000000, declared = " + fLength); //$NON-NLS-1$ } - int length = (int) rawLength; - byte[] bytes = new byte[length]; - for (int i = 0; i < length; i++) { + byte[] bytes = new byte[fLength]; + for (int i = 0; i < fLength; i++) { bytes[i] = (byte) input.get(Byte.SIZE, false); }ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/StructDeclaration.java (1)
55-61: Role map wiring is reasonable; consider clarifying key semanticsStoring roles in
fRolesfrom bothaddField(per-field roles) andsetRole(struct’s own role) provides the necessary backing for role-based lookups. Behavior is “last write wins” if a struct and one of its fields share the same role string.If that overlap is either impossible by design or significant when it happens, it may be worth documenting the intended semantics (struct vs. field precedence) or asserting on duplicate role keys. Otherwise the current implementation is fine.
Also applies to: 202-221, 348-352, 422-430
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
common/org.eclipse.tracecompass.common.core.tests/META-INF/MANIFEST.MF(1 hunks)common/org.eclipse.tracecompass.common.core.tests/src/org/eclipse/tracecompass/common/core/tests/log/LoggerTest.java(23 hunks)ctf/org.eclipse.tracecompass.ctf.core.tests/META-INF/MANIFEST.MF(1 hunks)ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/CTF2IntegrationTest.java(1 hunks)ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/FloatDeclarationParserTest.java(1 hunks)ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/JsonTraceMetadataNodeTest.java(1 hunks)ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/TypeAliasParserTest.java(1 hunks)ctf/org.eclipse.tracecompass.ctf.core/META-INF/MANIFEST.MF(1 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/metadata/DeclarationScope.java(3 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/BlobDeclaration.java(1 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/IntegerDeclaration.java(9 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/IntegerDefinition.java(2 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/ScopedDefinition.java(1 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/StringDefinition2.java(1 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/StructDeclaration.java(5 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/StructDefinition.java(4 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/VariantDeclaration.java(4 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/VariantDefinition.java(3 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/trace/Metadata.java(4 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/IOStructGen.java(3 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/JsonTraceMetadataNode.java(3 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/TypeAliasParser.java(4 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/dynamicarray/DynamicLengthArrayParser.java(1 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/dynamicstring/DynamicLengthStringParser.java(1 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/floatingpoint/FloatDeclarationParser.java(2 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/integer/IntegerDeclarationParser.java(1 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/staticarray/StaticLengthArrayParser.java(1 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/staticstring/StaticLengthStringParser.java(1 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/struct/StructDeclarationParser.java(4 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/struct/StructParser.java(2 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/variant/VariantParser.java(3 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/types/DynamicLengthStringDeclaration.java(1 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/types/StaticLengthStringDeclaration.java(1 hunks)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/utils/JsonMetadataStrings.java(5 hunks)ctf/org.eclipse.tracecompass.tmf.ctf.core/src/org/eclipse/tracecompass/tmf/ctf/core/event/CtfTmfEventField.java(2 hunks)ctf/org.eclipse.tracecompass.tmf.ctf.ui.swtbot.tests/src/org/eclipse/tracecompass/tmf/ctf/ui/swtbot/tests/TestInvalidCtfTrace.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/CTF2IntegrationTest.java
- ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/metadata/DeclarationScope.java
- ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/struct/StructDeclarationParser.java
- ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/VariantDeclaration.java
- ctf/org.eclipse.tracecompass.tmf.ctf.core/src/org/eclipse/tracecompass/tmf/ctf/core/event/CtfTmfEventField.java
- ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/TypeAliasParserTest.java
- ctf/org.eclipse.tracecompass.ctf.core.tests/META-INF/MANIFEST.MF
- ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/types/DynamicLengthStringDeclaration.java
- ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/StructDefinition.java
- ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/dynamicarray/DynamicLengthArrayParser.java
- ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/IntegerDefinition.java
- common/org.eclipse.tracecompass.common.core.tests/src/org/eclipse/tracecompass/common/core/tests/log/LoggerTest.java
🧰 Additional context used
🧬 Code graph analysis (8)
ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/FloatDeclarationParserTest.java (2)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/JsonStructureFieldMemberMetadataNode.java (1)
JsonStructureFieldMemberMetadataNode(31-115)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/floatingpoint/FloatDeclarationParser.java (1)
FloatDeclarationParser(66-206)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/struct/StructParser.java (4)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/JsonStructureFieldMemberMetadataNode.java (1)
JsonStructureFieldMemberMetadataNode(31-115)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/JsonStructureFieldMetadataNode.java (1)
JsonStructureFieldMetadataNode(29-89)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/AlignmentParser.java (1)
AlignmentParser(51-117)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/utils/JsonMetadataStrings.java (1)
JsonMetadataStrings(25-257)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/TypeAliasParser.java (7)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/dynamicarray/DynamicLengthArrayParser.java (1)
DynamicLengthArrayParser(33-115)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/dynamicstring/DynamicLengthStringParser.java (1)
DynamicLengthStringParser(46-102)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/enumeration/EnumParser.java (1)
EnumParser(97-266)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/staticarray/StaticLengthArrayParser.java (1)
StaticLengthArrayParser(33-112)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/staticstring/StaticLengthStringParser.java (1)
StaticLengthStringParser(46-102)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/struct/StructParser.java (1)
StructParser(95-285)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/utils/JsonMetadataStrings.java (1)
JsonMetadataStrings(25-257)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/variant/VariantParser.java (1)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/utils/JsonMetadataStrings.java (1)
JsonMetadataStrings(25-257)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/dynamicstring/DynamicLengthStringParser.java (3)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/TypeAliasParser.java (1)
NonNullByDefault(66-83)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/types/DynamicLengthStringDeclaration.java (1)
DynamicLengthStringDeclaration(30-116)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/utils/JsonMetadataStrings.java (1)
JsonMetadataStrings(25-257)
ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/JsonTraceMetadataNodeTest.java (1)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/JsonTraceMetadataNode.java (1)
JsonTraceMetadataNode(28-87)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/trace/Metadata.java (1)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/utils/JsonMetadataStrings.java (1)
JsonMetadataStrings(25-257)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/staticarray/StaticLengthArrayParser.java (2)
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/TypeAliasParser.java (2)
NonNullByDefault(66-83)TypeAliasParser(55-222)ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/utils/JsonMetadataStrings.java (1)
JsonMetadataStrings(25-257)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
🔇 Additional comments (17)
ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/FloatDeclarationParserTest.java (2)
45-53: Constructor fix confirmed—test correctly validates 32-bit float parsing.The constructor now correctly passes all 5 required parameters
(parent, type, value, name, fieldClass), resolving the critical compilation error flagged in previous reviews. The assertions properly verify thatFloatDeclarationParserinterpretslength: 32as 8-bit exponent and 24-bit mantissa with little-endian byte order.
70-79: Constructor fix confirmed—test correctly validates 64-bit float parsing.The constructor now correctly passes all 5 required parameters, resolving the critical issue from previous reviews. The assertions properly verify that
FloatDeclarationParserinterpretslength: 64as 11-bit exponent and 53-bit mantissa with big-endian byte order, plus the specified 8-byte alignment.ctf/org.eclipse.tracecompass.tmf.ctf.ui.swtbot.tests/src/org/eclipse/tracecompass/tmf/ctf/ui/swtbot/tests/TestInvalidCtfTrace.java (1)
89-89: Updated expected error string forstruct-duplicate-field-namelooks correctAligning the expected message with the new core duplicate-field wording (
"struct: duplicate field xxx") keeps this SWTBot test in sync with the parser’s behavior, and theassertContainsusage still gives a bit of flexibility. No further changes needed here.ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/trace/Metadata.java (1)
270-291: Element-based parsing after alias expansion is correctly implemented.The switch from
jsonBlocks[i]to the pre-parsed and alias-expandedelementensures that alias references are resolved before deserialization. The pattern is consistently applied across all fragment types.common/org.eclipse.tracecompass.common.core.tests/META-INF/MANIFEST.MF (1)
24-24: LGTM!The Gson import is correctly added to support the new JSON metadata handling in tests.
ctf/org.eclipse.tracecompass.ctf.core/META-INF/MANIFEST.MF (1)
5-5: LGTM!The minor version bump to 5.1.0 appropriately reflects the new CTF 2 support features being added.
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/floatingpoint/FloatDeclarationParser.java (1)
155-192: Legacy parsing path correctly preserved.The existing non-JSON metadata parsing logic is properly maintained within the else block, preserving backward compatibility with traditional TSDL metadata.
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/variant/VariantParser.java (1)
25-26: Good addition of IntegerDeclaration support for variant tags.The import and subsequent handling allow integer-based tags in addition to enum-based tags, which aligns with CTF 2 specification support.
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/IOStructGen.java (1)
159-171: Good integration of JSON-based environment extraction.The logic correctly checks for
JsonTraceMetadataNode, extracts the environment, and propagates it to the trace. The null check on line 162 is appropriate.ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/struct/StructParser.java (1)
192-222: Good addition of JSON-based struct parsing paths.The new branches correctly handle
JsonStructureFieldMetadataNodeandJsonStructureFieldMemberMetadataNodewith appropriate type checks (isJsonObject(),isJsonArray()). The minimum alignment extraction and member class iteration logic is well-structured.ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/JsonTraceMetadataNode.java (1)
19-67: JsonObject migration for environment looks consistentSwitching
fEnvironmentandgetEnvironment()toJsonObjectmatches the Gson-based metadata model and keeps behavior unchanged apart from the type. No issues from this change alone.ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/integer/IntegerDeclarationParser.java (1)
264-269: Mapped integer creation now usessizeconsistentlyUsing
(int) sizeas the first parameter forcreateDeclarationin the mappings path aligns it with the unmapped path and the updatedIntegerDeclarationfactory signature. This fixes the previous base/size mismatch.ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/VariantDefinition.java (1)
36-69: Variant tag widening and lookup integration look soundWidening
fTagDeftoSimpleDatatypeDefinitionwith a new constructor while retaining theEnumDefinitionconstructor keeps existing callers working and allows more generic tag types. The updatedlookupDefinitionmethods correctly:
- Match on the variant’s field name.
- Expose the tag and current field via their declaration roles.
- Delegate into the current field when it is a
ScopedDefinition, using the exclusion-aware overload.- Fall back to the enclosing
StructDefinition(or generic scope) while excluding this variant to avoid lookup loops.This is consistent with the new role-based lookup model.
Also applies to: 139-192
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/IntegerDeclaration.java (4)
134-143: LGTM: IntervalNode structure is clean and appropriate.The inner class provides a clear representation of interval boundaries with associated mapping names.
378-387: LGTM: Interval tree construction is correct.The buildIntervalTree implementation properly flattens all mapping ranges into sorted IntervalNodes.
549-549: LGTM: Using Long.SIZE is more explicit than a SIZE_64 constant.This improves code clarity by directly referencing the platform's long size.
567-567: LGTM: equals/hashCode properly updated for new fields.The inclusion of
fMappingsandgetRole()in equality and hashing logic maintains consistency with the new mapping functionality.Also applies to: 594-599
| @Override | ||
| public int getMaximumSize() { | ||
| return 1000000; | ||
| return fLength * 8; | ||
| } |
There was a problem hiding this comment.
Removed size limit could allow excessive memory allocation.
The previous implementation had a hard-coded maximum size of 1,000,000 that protected against excessively large allocations from malformed or malicious trace data. With this change, getMaximumSize() now returns fLength * 8, but the pre-flight guard in createDefinition that validated against this limit was also removed.
This means new byte[fLength] at line 66 will attempt to allocate whatever size is specified in the trace metadata without bounds checking, potentially causing OutOfMemoryError on corrupted or adversarial input.
Consider reintroducing a reasonable upper bound validation:
@Override
public @NonNull BlobDefinition createDefinition(IDefinitionScope definitionScope, @NonNull String fieldName, @NonNull BitBuffer input) throws CTFException {
+ if (fLength > 1_000_000) {
+ throw new CTFException("Blob size exceeds maximum allowed: " + fLength);
+ }
byte[] array = new byte[fLength];Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/BlobDeclaration.java
around lines 86-89 (and adjust the pre-flight check near createDefinition at
~line 66), reintroduce an upper-bound check to prevent unbounded allocations:
restore getMaximumSize to cap the reported maximum (e.g., return
Math.min(fLength * 8, SOME_REASONABLE_LIMIT)) and add a guard before allocating
new byte[fLength] that validates fLength against that same limit (or throws a
clear exception/logs and rejects the declaration) so malformed or malicious
trace metadata cannot trigger huge or OOM allocations; make the limit a named
constant (or configurable) and use it consistently for both the size report and
the allocation check.
| String getMappingForValue(long value) { | ||
| if (fIntervalTree.isEmpty()) { | ||
| return ""; //$NON-NLS-1$ | ||
| } | ||
| List<String> matches = new ArrayList<>(); | ||
| // Binary search for rightmost node with start <= value | ||
| int left = 0, right = fIntervalTree.size() - 1; | ||
| int lastValid = -1; | ||
| while (left <= right) { | ||
| int mid = (left + right) / 2; | ||
| if (fIntervalTree.get(mid).start <= value) { | ||
| lastValid = mid; | ||
| left = mid + 1; | ||
| } else { | ||
| right = mid - 1; | ||
| } | ||
| } | ||
| return mapping; | ||
| // Check all nodes from lastValid backwards for overlaps | ||
| for (int i = lastValid; i >= 0; i--) { | ||
| IntervalNode node = fIntervalTree.get(i); | ||
| if (node.end >= value) { | ||
| matches.add(node.name); | ||
| } | ||
| } | ||
| return matches.isEmpty() ? "" : Objects.requireNonNull(String.join(" ", matches)); //$NON-NLS-1$ //$NON-NLS-2$ | ||
| } |
There was a problem hiding this comment.
Potential duplicate names when the same mapping has multiple overlapping ranges.
The binary search and backward iteration logic is correct for finding overlapping intervals. However, if a single mapping name has multiple ranges that overlap with the queried value, that name will be added to matches multiple times.
Example scenario:
- Mapping:
"ERROR": [0-10], [5-15] - Query:
value = 7 - Result:
["ERROR", "ERROR"]→"ERROR ERROR"
Consider deduplicating matches using a LinkedHashSet to preserve order while avoiding duplicate names:
- List<String> matches = new ArrayList<>();
+ Set<String> matches = new LinkedHashSet<>();
// Binary search for rightmost node with start <= value
int left = 0, right = fIntervalTree.size() - 1;
int lastValid = -1;
while (left <= right) {
int mid = (left + right) / 2;
if (fIntervalTree.get(mid).start <= value) {
lastValid = mid;
left = mid + 1;
} else {
right = mid - 1;
}
}
// Check all nodes from lastValid backwards for overlaps
for (int i = lastValid; i >= 0; i--) {
IntervalNode node = fIntervalTree.get(i);
if (node.end >= value) {
matches.add(node.name);
}
}
return matches.isEmpty() ? "" : Objects.requireNonNull(String.join(" ", matches)); //$NON-NLS-1$ //$NON-NLS-2$📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| String getMappingForValue(long value) { | |
| if (fIntervalTree.isEmpty()) { | |
| return ""; //$NON-NLS-1$ | |
| } | |
| List<String> matches = new ArrayList<>(); | |
| // Binary search for rightmost node with start <= value | |
| int left = 0, right = fIntervalTree.size() - 1; | |
| int lastValid = -1; | |
| while (left <= right) { | |
| int mid = (left + right) / 2; | |
| if (fIntervalTree.get(mid).start <= value) { | |
| lastValid = mid; | |
| left = mid + 1; | |
| } else { | |
| right = mid - 1; | |
| } | |
| } | |
| return mapping; | |
| // Check all nodes from lastValid backwards for overlaps | |
| for (int i = lastValid; i >= 0; i--) { | |
| IntervalNode node = fIntervalTree.get(i); | |
| if (node.end >= value) { | |
| matches.add(node.name); | |
| } | |
| } | |
| return matches.isEmpty() ? "" : Objects.requireNonNull(String.join(" ", matches)); //$NON-NLS-1$ //$NON-NLS-2$ | |
| } | |
| String getMappingForValue(long value) { | |
| if (fIntervalTree.isEmpty()) { | |
| return ""; //$NON-NLS-1$ | |
| } | |
| Set<String> matches = new LinkedHashSet<>(); | |
| // Binary search for rightmost node with start <= value | |
| int left = 0, right = fIntervalTree.size() - 1; | |
| int lastValid = -1; | |
| while (left <= right) { | |
| int mid = (left + right) / 2; | |
| if (fIntervalTree.get(mid).start <= value) { | |
| lastValid = mid; | |
| left = mid + 1; | |
| } else { | |
| right = mid - 1; | |
| } | |
| } | |
| // Check all nodes from lastValid backwards for overlaps | |
| for (int i = lastValid; i >= 0; i--) { | |
| IntervalNode node = fIntervalTree.get(i); | |
| if (node.end >= value) { | |
| matches.add(node.name); | |
| } | |
| } | |
| return matches.isEmpty() ? "" : Objects.requireNonNull(String.join(" ", matches)); //$NON-NLS-1$ //$NON-NLS-2$ | |
| } |
🤖 Prompt for AI Agents
In
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/IntegerDeclaration.java
around lines 635-660, the current loop can add the same mapping name multiple
times when a name has multiple overlapping ranges; replace the List<String>
matches with a LinkedHashSet<String> to preserve insertion order and deduplicate
names as you collect matches, then join the set with a space for the return
(still returning an empty string if no matches).
| JsonStructureFieldMemberMetadataNode member = (JsonStructureFieldMemberMetadataNode) node; | ||
| JsonObject fieldClass = member.getFieldClass().getAsJsonObject(); |
There was a problem hiding this comment.
Missing type check before getAsJsonObject() call.
member.getFieldClass() returns a JsonElement which may not be a JsonObject. If the field class is a primitive or array, getAsJsonObject() will throw IllegalStateException.
JsonStructureFieldMemberMetadataNode member = (JsonStructureFieldMemberMetadataNode) node;
- JsonObject fieldClass = member.getFieldClass().getAsJsonObject();
+ JsonElement fieldClassElement = member.getFieldClass();
+ if (fieldClassElement == null || !fieldClassElement.isJsonObject()) {
+ throw new ParseException("Dynamic-length string field class must be a JSON object"); //$NON-NLS-1$
+ }
+ JsonObject fieldClass = fieldClassElement.getAsJsonObject();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| JsonStructureFieldMemberMetadataNode member = (JsonStructureFieldMemberMetadataNode) node; | |
| JsonObject fieldClass = member.getFieldClass().getAsJsonObject(); | |
| JsonStructureFieldMemberMetadataNode member = (JsonStructureFieldMemberMetadataNode) node; | |
| JsonElement fieldClassElement = member.getFieldClass(); | |
| if (fieldClassElement == null || !fieldClassElement.isJsonObject()) { | |
| throw new ParseException("Dynamic-length string field class must be a JSON object"); //$NON-NLS-1$ | |
| } | |
| JsonObject fieldClass = fieldClassElement.getAsJsonObject(); |
🤖 Prompt for AI Agents
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/dynamicstring/DynamicLengthStringParser.java
around lines 62-63: currently the code calls
member.getFieldClass().getAsJsonObject() without verifying the JsonElement is an
object; add a guard to call getAsJsonObject() only when member.getFieldClass()
is non-null and isJsonObject(), and handle other cases (primitive/array/null)
explicitly — e.g. skip processing, return a sensible default, or throw a clear
exception — so IllegalStateException is avoided and non-object fieldClass values
are handled safely.
| if (floatingPoint instanceof JsonStructureFieldMemberMetadataNode) { | ||
| JsonStructureFieldMemberMetadataNode member = (JsonStructureFieldMemberMetadataNode) floatingPoint; | ||
| JsonObject fieldclass = member.getFieldClass().getAsJsonObject(); | ||
|
|
||
| if (fieldclass.has(JsonMetadataStrings.LENGTH)) { | ||
| int size = fieldclass.get(JsonMetadataStrings.LENGTH).getAsInt(); | ||
| // Standard IEEE 754 sizes | ||
| if (size == 32) { | ||
| exponent = 8; | ||
| mantissa = 24; | ||
| } else if (size == 64) { | ||
| exponent = 11; | ||
| mantissa = 53; | ||
| } else { | ||
| throw new ParseException("Unsupported floating point size: " + size); | ||
| } | ||
| String left = concatenateUnaryStrings(leftStrings); | ||
| if (left.equals(MetadataStrings.EXP_DIG)) { | ||
| exponent = UnaryIntegerParser.INSTANCE.parse(rightNode.getChild(0), null).intValue(); | ||
| } else if (left.equals(MetadataStrings.BYTE_ORDER)) { | ||
| byteOrder = ByteOrderParser.INSTANCE.parse(rightNode, new ByteOrderParser.Param(trace)); | ||
| } else if (left.equals(MetadataStrings.MANT_DIG)) { | ||
| mantissa = UnaryIntegerParser.INSTANCE.parse(rightNode.getChild(0), null).intValue(); | ||
| } else if (left.equals(MetadataStrings.ALIGN)) { | ||
| alignment = AlignmentParser.INSTANCE.parse(rightNode, null); | ||
| } | ||
|
|
||
| if (fieldclass.has(JsonMetadataStrings.BYTE_ORDER)) { | ||
| CTFJsonMetadataNode bo = new CTFJsonMetadataNode(floatingPoint, CTFParser.tokenNames[CTFParser.UNARY_EXPRESSION_STRING], fieldclass.get(JsonMetadataStrings.BYTE_ORDER).getAsString()); | ||
| byteOrder = ByteOrderParser.INSTANCE.parse(bo, new ByteOrderParser.Param(trace)); | ||
| } | ||
|
|
||
| if (fieldclass.has(JsonMetadataStrings.ALIGNMENT)) { | ||
| alignment = fieldclass.get(JsonMetadataStrings.ALIGNMENT).getAsInt(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/floatingpoint && wc -l FloatDeclarationParser.javaRepository: eclipse-tracecompass/org.eclipse.tracecompass
Length of output: 120
🏁 Script executed:
cd ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/floatingpoint && sed -n '100,200p' FloatDeclarationParser.javaRepository: eclipse-tracecompass/org.eclipse.tracecompass
Length of output: 4657
🏁 Script executed:
cd ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/floatingpoint && head -50 FloatDeclarationParser.javaRepository: eclipse-tracecompass/org.eclipse.tracecompass
Length of output: 2814
🌐 Web query:
CTF 2 specification floating point field class supported sizes
💡 Result:
CTF 2 fixed-length floating-point field classes support bit-lengths 16, 32, 64, 128 — and also K where K > 128 and K is a multiple of 32. [1][2]
Sources:
[1] CTF2 SPECRC (Fixed-length floating point number field class).
[2] CTF2 spec (Fixed-length floating point number field class).
JSON parsing path is overly restrictive and incompatible with CTF 2 specification.
The code only supports 32-bit and 64-bit floating point sizes in the JSON path, rejecting all others. However, CTF 2 specification requires support for 16-bit, 32-bit, 64-bit, 128-bit, and arbitrary K-bit (where K > 128 and is a multiple of 32) field classes. The current implementation should be extended to handle all these required sizes.
The byte order parsing correctly uses the JsonMetadataStrings.BYTE_ORDER constant.
🤖 Prompt for AI Agents
In
ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/floatingpoint/FloatDeclarationParser.java
around lines 129-154, the size handling currently accepts only 32 and 64 bits;
update it to accept 16, 32, 64, 128 and any K > 128 that is a multiple of 32.
Map known sizes to exponent/mantissa pairs (16 -> exponent=5, mantissa=11; 32 ->
exponent=8, mantissa=24; 64 -> exponent=11, mantissa=53; 128 -> exponent=15,
mantissa=113) and for K > 128 where K % 32 == 0 compute mantissa = K - 1 -
exponent and use exponent = 15 (or the spec-required exponent for large formats)
so mantissa = K - 1 - 15; keep the same unsupported-size ParseException for any
other values. Also leave byte order and alignment parsing unchanged.
|
@bhufmann @PatrickTasse Do you need any help for the review? |
Yes, if you could do the review that would be great. Then I can focus on some other tasks. @arfio thanks for offering. |
3 participants
What it does
How to test
Try to open eclipse-tracecompass/tracecompass-test-traces#12 the traces there
Follow-ups
Review checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Version Update
✏️ Tip: You can customize this high-level summary in your review settings.