Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ads] size_t code health #27711

Merged
merged 1 commit into from
Feb 19, 2025
Merged

[ads] size_t code health #27711

merged 1 commit into from
Feb 19, 2025

Conversation

tmancey
Copy link
Collaborator

@tmancey tmancey commented Feb 18, 2025

Resolves brave/brave-browser#44064

Submitter Checklist:

  • I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally:
    • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
    • npm run presubmit wiki, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

@tmancey tmancey self-assigned this Feb 18, 2025
@tmancey tmancey requested a review from a team as a code owner February 18, 2025 16:49
Copy link
Contributor

[puLL-Merge] - brave/brave-core@27711

Description

This PR changes integer types across the Brave Ads codebase to use more appropriate and safer types, specifically:

  1. Changes various integer parameters and counters from int to size_t for array indices, sizes and counts
  2. Changes database indices from int to int32_t for explicit sizing
  3. Adds cstddef and cstdint headers where needed
  4. Updates associated tests and feature parameters to use the correct types

The motivation appears to be to improve type safety and prevent potential integer overflow issues by using more appropriate types for different use cases.

Changes

Changes

By filename:

  1. Multiple database table files:
  • Changed index types from int to int32_t for database operations
  • Added cstdint header
  1. Feature files:
  • Changed feature parameters from int to size_t for counts/sizes
  • Updated tests to use unsigned comparisons
  1. Test utility files:
  • Changed count parameters from int to size_t
  • Updated loops to use size_t for iteration
  • Added bounds checking with unsigned comparisons
  1. Account management files:
  • Updated token counts and limits to use size_t
  • Changed array indices to use size_t
  1. User activity files:
  • Changed event counters to use size_t
  • Updated maximum event count comparisons
sequenceDiagram
    participant Code
    participant Type System
    participant Compiler
    
    Code->>Type System: Change int to size_t for counts
    Type System->>Compiler: Enforce unsigned bounds
    Compiler->>Code: Prevent integer overflow
    
    Code->>Type System: Change int to int32_t for DB
    Type System->>Compiler: Enforce explicit sizing
    Compiler->>Code: Ensure consistent DB types
Loading

Possible Issues

  1. Some code paths may need to handle the case where size_t wraps around to 0, particularly in arithmetic operations
  2. Interface changes from signed to unsigned types may require updating consumer code to handle the type changes properly

The main goal appears to be using more appropriate types for different use cases, which should improve type safety and prevent potential overflow issues.

@tmancey tmancey force-pushed the issues/44064 branch 3 times, most recently from f681cdd to 94d1c84 Compare February 18, 2025 17:32
@tmancey tmancey enabled auto-merge (squash) February 18, 2025 17:36
Copy link
Contributor

Chromium major version is behind target branch (133.0.6943.98 vs 134.0.6998.15). Please rebase.

@github-actions github-actions bot added the chromium-version-mismatch The Chromium version on the PR branch does not match the version on the target branch label Feb 18, 2025
@github-actions github-actions bot removed the chromium-version-mismatch The Chromium version on the PR branch does not match the version on the target branch label Feb 18, 2025
@tmancey tmancey merged commit b91c7f1 into master Feb 19, 2025
18 checks passed
@tmancey tmancey deleted the issues/44064 branch February 19, 2025 14:36
@github-actions github-actions bot added this to the 1.77.x - Nightly milestone Feb 19, 2025
@brave-builds
Copy link
Collaborator

Released in v1.77.53

kdenhartog pushed a commit that referenced this pull request Feb 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ads] size_t code health
3 participants