ctf: Add new field classes to complete ctf2 parsing#400
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (17)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (13)
📝 WalkthroughWalkthroughAdds fixed-length boolean and dynamic-length blob types with declarations/definitions, TSDL parsers, TypeAlias wiring, event-field rendering (Base64 with media type), unit tests, JSON metadata constants, and bumps bundle version to 5.2.0. ChangesCTF Type System Expansion: Boolean and Dynamic Blob Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
ctf/org.eclipse.tracecompass.tmf.ctf.core/src/org/eclipse/tracecompass/tmf/ctf/core/event/CtfTmfEventField.java (1)
583-599: ⚡ Quick winCache formatted blob output to avoid repeated Base64 work.
getFormattedValue()re-encodes bytes on every call; with dynamic blobs this can become expensive in repeated rendering paths. Cache the formatted string once (same pattern already used by other field classes here).♻️ Proposed refactor
final class CTFHexBlobField extends CtfTmfEventField { - private String fBlobType; + private final String fBlobType; + private `@Nullable` String fFormattedValue; @@ `@Override` - public String getFormattedValue() { - String encoded = Base64.getEncoder().encodeToString(getValue()); - return "[" + fBlobType + "]<" + encoded + '>'; //$NON-NLS-1$ //$NON-NLS-2$ + public synchronized String getFormattedValue() { + if (fFormattedValue == null) { + String encoded = Base64.getEncoder().encodeToString(getValue()); + fFormattedValue = "[" + fBlobType + "]<" + encoded + '>'; //$NON-NLS-1$ //$NON-NLS-2$ + } + return fFormattedValue; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ctf/org.eclipse.tracecompass.tmf.ctf.core/src/org/eclipse/tracecompass/tmf/ctf/core/event/CtfTmfEventField.java` around lines 583 - 599, getFormattedValue() currently Base64-encodes getValue() on every call; add a lazily-initialized cached String field (e.g., fFormattedValue) on CTFHexBlobField and compute the "[" + fBlobType + "]<" + Base64.getEncoder().encodeToString(getValue()) + '>' only once in getFormattedValue(), store it in the cache and return the cached value on subsequent calls; follow the same lazy-cache pattern and naming conventions used by other field classes, and make the cache safe for concurrent reads (e.g., a volatile field or simple double-check idiom) so repeated rendering avoids repeated Base64 work.ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/DynamicBlobDefinition.java (1)
58-60: ⚡ Quick winReturn a defensive copy from
getBytes().
getBytes()currently exposes internal mutable state. Any caller can mutatefArrayand alter this definition instance unexpectedly.Proposed fix
`@Override` public byte[] getBytes() { - return fArray; + return fArray.clone(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/DynamicBlobDefinition.java` around lines 58 - 60, DynamicBlobDefinition.getBytes exposes the internal mutable byte[] fArray; change getBytes() to return a defensive copy (e.g., clone or Arrays.copyOf) of fArray instead of the array reference, and handle the null case consistently (return null or an empty array as appropriate) so callers cannot mutate the internal state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/BooleanDeclaration.java`:
- Around line 50-55: In createDefinition, ensure the original ByteOrder
(variable byteOrder) is always restored on the BitBuffer even if read(input)
throws: wrap the call to read(input) (and any code that depends on the temporary
fByteOrder) in a try/finally and call input.setByteOrder(byteOrder) in the
finally block so the buffer state is restored before returning or propagating
the exception; reference createDefinition, read(Input)/read(input), fByteOrder,
input.setByteOrder(...), and BooleanDefinition to locate the change.
- Around line 58-63: The BooleanDeclaration.read method incorrectly increments
the chunk index by 1 bit; change the loop to advance by 64-bit chunks and
compute the remaining bits per iteration. Specifically, in read(BitBuffer input)
update the for-loop to iterate with "for (int i = 0; i < fLength; i += 64)" and
use "int remaining = fLength - i" then call input.get(Math.min(64, remaining),
false) so BitBuffer.get is always passed a valid chunk length and the reader
advances by full 64-bit chunks.
In
`@ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/DynamicBlobDeclaration.java`:
- Around line 41-43: The constructor DynamicBlobDeclaration currently accepts a
nullable lengthName and stores it in fLengthName, but later code (including
other occurrences around the constructor and at the usages noted) dereferences
or compares fLengthName directly; modify all comparisons and dereferences of
fLengthName (including in methods referenced near lines 51-52 and 122-123) to
handle null safely by using null-safe checks (e.g., check fLengthName != null
before calling equals/compare, or use Objects.equals(fLengthName, other) /
Objects.equals(other, fLengthName)) and avoid direct method calls on fLengthName
when it may be null, ensuring the constructor and any equality/lookup logic
account for a null lengthName value.
In
`@ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/DynamicBlobDeclarationParser.java`:
- Around line 72-79: The code currently dereferences
lengthFieldLocation/length-field-location.path without validation in
DynamicBlobDeclarationParser, which can cause runtime exceptions; update the
parsing logic to first confirm lengthFieldLocation.isJsonObject() and that the
object has JsonMetadataStrings.PATH, then ensure the path element is either a
non-empty JsonArray whose last element is a JsonPrimitive string or a
JsonPrimitive string; if any check fails (missing path, path not
object/array/string, empty array, or non-string last element) throw a
ParseException with a clear message; replace direct calls to
getAsJsonObject/getAsJsonArray/getAsString for
lengthFieldLocation/pathElement/lengthName with guarded checks that produce
deterministic ParseException errors.
In
`@ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/TypeAliasParser.java`:
- Around line 196-201: The switch branches for
JsonMetadataStrings.FIXED_LENGTH_BIT_ARRAY and
JsonMetadataStrings.FIXED_LENGTH_BIT_MAP in TypeAliasParser currently break
without assigning targetDeclaration, which later causes a null registration;
change these branches to throw a ParseException (with a clear message mentioning
the unsupported field class) instead of breaking so parsing fails fast. Locate
the switch handling field class in TypeAliasParser (the cases for
FIXED_LENGTH_BIT_ARRAY and FIXED_LENGTH_BIT_MAP) and replace the break logic
with code that raises a ParseException referencing the offending field class and
the alias name being processed, ensuring the method never reaches the later
register-type-alias path with a null targetDeclaration.
---
Nitpick comments:
In
`@ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/DynamicBlobDefinition.java`:
- Around line 58-60: DynamicBlobDefinition.getBytes exposes the internal mutable
byte[] fArray; change getBytes() to return a defensive copy (e.g., clone or
Arrays.copyOf) of fArray instead of the array reference, and handle the null
case consistently (return null or an empty array as appropriate) so callers
cannot mutate the internal state.
In
`@ctf/org.eclipse.tracecompass.tmf.ctf.core/src/org/eclipse/tracecompass/tmf/ctf/core/event/CtfTmfEventField.java`:
- Around line 583-599: getFormattedValue() currently Base64-encodes getValue()
on every call; add a lazily-initialized cached String field (e.g.,
fFormattedValue) on CTFHexBlobField and compute the "[" + fBlobType + "]<" +
Base64.getEncoder().encodeToString(getValue()) + '>' only once in
getFormattedValue(), store it in the cache and return the cached value on
subsequent calls; follow the same lazy-cache pattern and naming conventions used
by other field classes, and make the cache safe for concurrent reads (e.g., a
volatile field or simple double-check idiom) so repeated rendering avoids
repeated Base64 work.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 26a2a8a9-0946-48b0-805a-b76f0a0e7a86
📒 Files selected for processing (13)
ctf/org.eclipse.tracecompass.ctf.core/META-INF/MANIFEST.MFctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/BlobDeclaration.javactf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/BlobDefinition.javactf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/BooleanDeclaration.javactf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/BooleanDefinition.javactf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/DynamicBlobDeclaration.javactf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/DynamicBlobDefinition.javactf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/BlobDeclarationParser.javactf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/DynamicBlobDeclarationParser.javactf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/TypeAliasParser.javactf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/bool/BooleanDeclarationParser.javactf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/utils/JsonMetadataStrings.javactf/org.eclipse.tracecompass.tmf.ctf.core/src/org/eclipse/tracecompass/tmf/ctf/core/event/CtfTmfEventField.java
MatthewKhouzam
approved these changes
May 22, 2026
MatthewKhouzam
previously approved these changes
May 22, 2026
MatthewKhouzam
left a comment
MatthewKhouzam
left a comment
Contributor
There was a problem hiding this comment.
Looks great. add unit tests.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/DynamicBlobDeclarationTest.java (1)
42-97: ⚡ Quick winAdd failure-path tests for guard clauses.
Current coverage is solid for success cases, but missing tests for
CTFExceptionpaths (missing length field, signed length declaration, and insufficient buffer after alignment). Adding those would lock in the contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/DynamicBlobDeclarationTest.java` around lines 42 - 97, Add unit tests to cover the three failure paths that currently lack coverage: when the referenced length field is missing, when the length field declaration is signed, and when the buffer doesn't contain enough bytes after alignment. In DynamicBlobDeclarationTest add three `@Test` methods that construct the same test harness (use IntegerDeclaration.createDeclaration, StructDeclaration/StructDefinition, BitBuffer) but for each case assert that DynamicBlobDeclaration.createDefinition(...) throws CTFException: (1) omit adding the length field to StructDeclaration to trigger "missing length field", (2) use an IntegerDeclaration with signed=true to trigger the "signed length" guard, and (3) create a ByteBuffer/BitBuffer with too-small bufferSize/startingPosition to trigger the "insufficient buffer after alignment" guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/DynamicBlobDeclaration.java`:
- Around line 68-79: The bounds checks in DynamicBlobDeclaration are using bits
vs bytes incorrectly and check remaining bytes before alignment; fix by first
calling alignRead(input), then verify input.canRead((int)length * 8) (canRead
expects bit count), ensure length does not exceed Integer.MAX_VALUE as before,
and then check input.getByteBuffer().remaining() >= length (bytes) before
allocating the byte[] and calling input.get(array); update the thrown
CTFException messages to reflect the correct units (bits vs bytes) and include
the same context used in input.get(array).
---
Nitpick comments:
In
`@ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/DynamicBlobDeclarationTest.java`:
- Around line 42-97: Add unit tests to cover the three failure paths that
currently lack coverage: when the referenced length field is missing, when the
length field declaration is signed, and when the buffer doesn't contain enough
bytes after alignment. In DynamicBlobDeclarationTest add three `@Test` methods
that construct the same test harness (use IntegerDeclaration.createDeclaration,
StructDeclaration/StructDefinition, BitBuffer) but for each case assert that
DynamicBlobDeclaration.createDefinition(...) throws CTFException: (1) omit
adding the length field to StructDeclaration to trigger "missing length field",
(2) use an IntegerDeclaration with signed=true to trigger the "signed length"
guard, and (3) create a ByteBuffer/BitBuffer with too-small
bufferSize/startingPosition to trigger the "insufficient buffer after alignment"
guard.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e599f89a-4a7e-4cc7-b59b-80bb0754afce
📒 Files selected for processing (17)
ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/BooleanDeclarationParserTest.javactf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/BooleanDeclarationTest.javactf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/BooleanDefinitionTest.javactf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/DynamicBlobDeclarationTest.javactf/org.eclipse.tracecompass.ctf.core/META-INF/MANIFEST.MFctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/BlobDeclaration.javactf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/BlobDefinition.javactf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/BooleanDeclaration.javactf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/BooleanDefinition.javactf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/DynamicBlobDeclaration.javactf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/DynamicBlobDefinition.javactf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/BlobDeclarationParser.javactf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/DynamicBlobDeclarationParser.javactf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/TypeAliasParser.javactf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/bool/BooleanDeclarationParser.javactf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/utils/JsonMetadataStrings.javactf/org.eclipse.tracecompass.tmf.ctf.core/src/org/eclipse/tracecompass/tmf/ctf/core/event/CtfTmfEventField.java
✅ Files skipped from review due to trivial changes (1)
- ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/BlobDeclaration.java
🚧 Files skipped from review as they are similar to previous changes (12)
- ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/BlobDeclarationParser.java
- ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/DynamicBlobDefinition.java
- ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/utils/JsonMetadataStrings.java
- ctf/org.eclipse.tracecompass.tmf.ctf.core/src/org/eclipse/tracecompass/tmf/ctf/core/event/CtfTmfEventField.java
- ctf/org.eclipse.tracecompass.ctf.core/META-INF/MANIFEST.MF
- ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/BooleanDefinitionTest.java
- ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/BooleanDeclarationTest.java
- ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/bool/BooleanDeclarationParser.java
- ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/BooleanDefinition.java
- ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/DynamicBlobDeclarationParser.java
- ctf/org.eclipse.tracecompass.ctf.core.tests/src/org/eclipse/tracecompass/ctf/core/tests/types/BooleanDeclarationParserTest.java
- ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/TypeAliasParser.java
Signed-off-by: Arnaud Fiorini <fiorini.arnaud@gmail.com>
Signed-off-by: Arnaud Fiorini <fiorini.arnaud@gmail.com>
MatthewKhouzam
approved these changes
May 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What it does
How to test
Follow-ups
Review checklist
Summary by CodeRabbit
New Features
Documentation / Parsing
Tests