Skip to content

fix(dart): use getUint32 for correctly encoding u64 value#3592

Merged
chaokunyang merged 4 commits intoapache:mainfrom
ayush00git:fix/dart-buffer-tui64
Apr 20, 2026
Merged

fix(dart): use getUint32 for correctly encoding u64 value#3592
chaokunyang merged 4 commits intoapache:mainfrom
ayush00git:fix/dart-buffer-tui64

Conversation

@ayush00git
Copy link
Copy Markdown
Contributor

@ayush00git ayush00git commented Apr 20, 2026

Why?

getInt32 treats the top bit as a sign bit. For large values where that bit is set (0xFFFFFFFE), the number comes back negative. Even though >>> is unsigned shift, it is shifting the already-corrupted signed value, garbage in, garbage out.

What does this PR do?

Use getUint32 instead.
getUint32 has no sign bit, so all 32 bits are treated as magnitude.

Related issues

N/A

AI Contribution Checklist

  • Substantial AI assistance was used in this PR: yes / no
  • If yes, I included a completed AI Contribution Checklist in this PR description and the required AI Usage Disclosure.
  • If yes, my PR description includes the required ai_review summary and screenshot evidence of the final clean AI review results from both fresh reviewers on the current PR diff or current HEAD after the latest code changes.

Does this PR introduce any user-facing change?

  • Does this PR introduce any public API change?
  • Does this PR introduce any binary protocol compatibility change?

Benchmark

@ayush00git
Copy link
Copy Markdown
Contributor Author

@chaokunyang
why do the dart files misses the apache LICENSE ?

int readTaggedUint64() {
final readIndex = _readerIndex;
final first = _view.getInt32(readIndex, Endian.little);
final first = _view.getUint32(readIndex, Endian.little);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes Buffer.readTaggedUint64(), but GeneratedReadCursor.readTaggedUint64() in dart/packages/fory/lib/src/codegen/generated_support.dart still reads the first word with getInt32. Generated serializers route tagged uint64 fields through that cursor path, so 0x7fffffff still round-trips there as 9223372036854775807 instead of 2147483647. The generated path needs the same getUint32 change and a regression test.

@chaokunyang
Copy link
Copy Markdown
Collaborator

@chaokunyang why do the dart files misses the apache LICENSE ?

I missed that, let me add it

chaokunyang added a commit that referenced this pull request Apr 20, 2026
## Why?



## What does this PR do?



## Related issues

#3592 

## AI Contribution Checklist



- [ ] Substantial AI assistance was used in this PR: `yes` / `no`
- [ ] If `yes`, I included a completed [AI Contribution
Checklist](https://github.com/apache/fory/blob/main/AI_POLICY.md#9-contributor-checklist-for-ai-assisted-prs)
in this PR description and the required `AI Usage Disclosure`.
- [ ] If `yes`, my PR description includes the required `ai_review`
summary and screenshot evidence of the final clean AI review results
from both fresh reviewers on the current PR diff or current HEAD after
the latest code changes.



## Does this PR introduce any user-facing change?



- [ ] Does this PR introduce any public API change?
- [ ] Does this PR introduce any binary protocol compatibility change?

## Benchmark
@ayush00git
Copy link
Copy Markdown
Contributor Author

I missed that, let me add it

@chaokunyang
also aren't we following any linting format for dart files?

@chaokunyang
Copy link
Copy Markdown
Collaborator

I missed that, let me add it

@chaokunyang also aren't we following any linting format for dart files?

it seems licenserc doesn't support check dart, could you check it?

Copy link
Copy Markdown
Collaborator

@chaokunyang chaokunyang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@chaokunyang chaokunyang merged commit b4d7ad3 into apache:main Apr 20, 2026
62 checks passed
@ayush00git
Copy link
Copy Markdown
Contributor Author

it seems licenserc doesn't support check dart, could you check it?

@chaokunyang
I was actually asking about code formatting (like dart format) and linting (dart analyze), not the license headers. we already run dart analyze in ci, but we don't enforce dart format yet. should we add a formatting check?

and for the licenserc issue, we just need to add the .dart extension to licenserc.toml so the ci explicitly enforces it. should i add them ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants