Skip to content

Remove atomics section from codereview_guideline.md#53018

Open
rahulkaukuntla wants to merge 1 commit into
mainfrom
rahul/remove-atomic-codereview
Open

Remove atomics section from codereview_guideline.md#53018
rahulkaukuntla wants to merge 1 commit into
mainfrom
rahul/remove-atomic-codereview

Conversation

@rahulkaukuntla

Copy link
Copy Markdown
Contributor

What does this PR do?

Due to golang/go#50860, with the adoption of go version 1.19:

The sync/atomic package defines new atomic types Bool, Int32, Int64, Uint32, Uint64, Uintptr, and Pointer. These types hide the underlying values so that all accesses are forced to use the atomic APIs. Int64 and Uint64 are automatically aligned to 64-bit boundaries in structs and allocated data, even on 32-bit systems.

This repo is already >1.19, so we don't need to move over to the uber atomic library.

Motivation

Describe how you validated your changes

Additional Notes

@rahulkaukuntla rahulkaukuntla requested a review from a team as a code owner June 30, 2026 20:27
@dd-octo-sts dd-octo-sts Bot added internal Identify a non-fork PR team/agent-devx labels Jun 30, 2026
@github-actions github-actions Bot added the short review PR is simple enough to be reviewed quickly label Jun 30, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 02e5c10854

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread codereview_guideline.md
to guarantee alignment, unless placed as the first field with a comment
explaining why a pointer was not used.

### Testing: `require` vs `assert`

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Keep coverage for primitive atomic APIs

This deletion removes the reviewer-facing warning about sync/atomic, but the Go 1.19 alignment fix only covers the new typed values; primitive 64-bit functions such as atomic.AddUint64 still require caller-arranged alignment on ARM/386/32-bit MIPS, and the public Go guideline still tells contributors to use go.uber.org/atomic instead (docs/public/guidelines/languages/go.md:122-130). With this gone, a PR adding primitive sync/atomic on a struct field can pass review while remaining broken on supported 32-bit/ARM targets, so please replace this with a narrower rule for primitive APIs rather than removing it outright.

Useful? React with 👍 / 👎.

@rahulkaukuntla rahulkaukuntla added changelog/no-changelog No changelog entry needed qa/done QA done before merge and regressions are covered by tests labels Jun 30, 2026
@dd-octo-sts

dd-octo-sts Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Files inventory check summary

File checks results against ancestor cf57354d:

Results for datadog-agent_7.82.0~devel.git.498.02e5c10.pipeline.121972054-1_amd64.deb:

No change detected

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

Labels

changelog/no-changelog No changelog entry needed internal Identify a non-fork PR qa/done QA done before merge and regressions are covered by tests short review PR is simple enough to be reviewed quickly team/agent-devx

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant