Skip to content

[CODE HEALTH] Fix clang-tidy narrowing-conversions warnings in otlp_populate_attribute_utils#4090

Open
thc1006 wants to merge 1 commit into
open-telemetry:mainfrom
thc1006:codehealth/otlp-narrowing-nolint-alias
Open

[CODE HEALTH] Fix clang-tidy narrowing-conversions warnings in otlp_populate_attribute_utils#4090
thc1006 wants to merge 1 commit into
open-telemetry:mainfrom
thc1006:codehealth/otlp-narrowing-nolint-alias

Conversation

@thc1006
Copy link
Copy Markdown
Contributor

@thc1006 thc1006 commented May 18, 2026

Summary

After the clang-tidy v22 upgrade in #4066, four narrowing-conversion warnings surfaced in exporters/otlp/src/otlp_populate_attribute_utils.cc, at the four uint64_t to int64_t call sites passing values to proto set_int_value:

Line Branch
~78 scalar uint64_t (PopulateAnyValue first overload)
~169 nostd::span<const uint64_t> array
~234 scalar uint64_t (PopulateAnyValue second overload)
~312 std::vector<uint64_t> array

Why static_cast, not a NOLINT rename

An earlier attempt to fix these by renaming the existing NOLINT(cppcoreguidelines-narrowing-conversions) to the new bugprone- alias name did not work: in clang-tidy v22 both names are registered as separate aliases, and NOLINT for one alias does not suppress reporting under the other. The warning simply flips to the other alias name. A single-name NOLINT cannot silence the diagnostic.

This PR instead replaces the four implicit narrowings with explicit static_cast<int64_t> conversions, matching the pattern established in the prior narrowing-cleanup PRs #3989, #3997, and #4013. This eliminates the alias dependency entirely and makes each narrowing explicit at the call site. No runtime behavior change — implicit narrowing under two's-complement was already producing the same bit pattern.

A fifth NOLINT at line ~297 (a uint32_t to int64_t conversion, which is signed widening rather than narrowing) is left untouched. clang-tidy v22 emits no warning there, and removing the dead NOLINT is out of scope.

Ratchet

  • all-options-abiv1-preview warning_limit lowered from 418 to 414
  • all-options-abiv2-preview warning_limit lowered from 424 to 420

Verification

Counts measured against artifacts of the most recent successful CI run on main (workflow clang-tidy.yaml, run 26018587378):

Preset Before (count / limit) After (predicted)
abiv1-preview 418 / 418 414 / 414
abiv2-preview 424 / 424 420 / 420

Test plan

  • CI clang-tidy job passes both abiv1-preview and abiv2-preview at the new limits
  • CI Format job passes (single-line static_cast expressions fit within 100 chars)
  • No behavioral change to OtlpPopulateAttributeUtils::PopulateAnyValue; existing tests in exporters/otlp/test/otlp_recordable_test.cc still cover the modified branches via type-parameterized tests over uint64_t

Part of #2053

Copilot AI review requested due to automatic review settings May 18, 2026 13:31
@thc1006 thc1006 requested a review from a team as a code owner May 18, 2026 13:31
@thc1006 thc1006 force-pushed the codehealth/otlp-narrowing-nolint-alias branch from b15a543 to e9fc36b Compare May 18, 2026 13:32
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Updates OTLP exporter code to align existing clang-tidy suppression comments with the clang-tidy v22 check name, and adjusts CI warning limits accordingly. This is a code health/CI maintenance change intended to keep the clang-tidy ratchet stable after the LLVM/clang-tidy 22 upgrade.

Changes:

  • Renames four NOLINT suppressions in otlp_populate_attribute_utils.cc from the old alias to bugprone-narrowing-conversions.
  • Lowers clang-tidy workflow warning limits for all-options-abiv1-preview and all-options-abiv2-preview.
  • Adds a corresponding CHANGELOG.md entry.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
exporters/otlp/src/otlp_populate_attribute_utils.cc Updates four narrowing-conversion NOLINT suppressions to match clang-tidy v22’s check name.
CHANGELOG.md Adds an Unreleased entry for the code health change.
.github/workflows/clang-tidy.yaml Lowers the warning limits to reflect the reduced warning count.

Comment thread CHANGELOG.md Outdated
@thc1006 thc1006 force-pushed the codehealth/otlp-narrowing-nolint-alias branch from e9fc36b to cc4c581 Compare May 18, 2026 14:16
@thc1006 thc1006 changed the title [CODE HEALTH] Update bugprone-narrowing-conversions NOLINT suppressions in otlp_populate_attribute_utils [CODE HEALTH] Fix clang-tidy narrowing-conversions warnings in otlp_populate_attribute_utils May 18, 2026
@thc1006 thc1006 requested a review from Copilot May 18, 2026 14:18
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

Comment thread exporters/otlp/src/otlp_populate_attribute_utils.cc
{
proto_value->set_int_value(
nostd::get<uint64_t>(value)); // NOLINT(cppcoreguidelines-narrowing-conversions)
proto_value->set_int_value(static_cast<int64_t>(nostd::get<uint64_t>(value)));
{
array_value->add_values()->set_int_value(
val); // NOLINT(cppcoreguidelines-narrowing-conversions)
array_value->add_values()->set_int_value(static_cast<int64_t>(val));
{
array_value->add_values()->set_int_value(
val); // NOLINT(cppcoreguidelines-narrowing-conversions)
array_value->add_values()->set_int_value(static_cast<int64_t>(val));
@thc1006 thc1006 force-pushed the codehealth/otlp-narrowing-nolint-alias branch from cc4c581 to 2c3d074 Compare May 18, 2026 14:32
@codecov
Copy link
Copy Markdown

codecov Bot commented May 18, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.99%. Comparing base (026af08) to head (785ec9a).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4090      +/-   ##
==========================================
- Coverage   81.99%   81.99%   -0.00%     
==========================================
  Files         385      385              
  Lines       16023    16019       -4     
==========================================
- Hits        13137    13133       -4     
  Misses       2886     2886              
Files with missing lines Coverage Δ
...xporters/otlp/src/otlp_populate_attribute_utils.cc 92.71% <100.00%> (-0.20%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

thc1006 added a commit to thc1006/opentelemetry-cpp that referenced this pull request May 18, 2026
Eliminate the 29 misc-unused-alias-decls warnings flagged by clang-tidy
across 27 files in examples, exporters, sdk, and tests. The most common
pattern was a file-scope `namespace nostd = opentelemetry::nostd;`
declaration that was never referenced via the file-scope alias path
(uses of `nostd::` resolved through the surrounding
OPENTELEMETRY_BEGIN_NAMESPACE namespace nesting instead).

No behavior change. Removed declarations were genuinely dead per clang-tidy.
Where removing an alias affected sibling-line alignment (e.g. `using M = ...`
next to the removed alias), clang-format auto-collapsed the alignment.

Ratchet:
* abiv1-preview warning_limit lowered from 418 to 389
* abiv2-preview warning_limit lowered from 424 to 395

A 30th misc-unused-alias-decls site at
exporters/otlp/src/otlp_populate_attribute_utils.cc:30 is intentionally
not touched in this PR to avoid conflicting with the in-flight open-telemetry#4090.
It can be cleaned up in a follow-up after open-telemetry#4090 lands.

Part of open-telemetry#2053

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
thc1006 added a commit to thc1006/opentelemetry-cpp that referenced this pull request May 18, 2026
Eliminate 29 misc-unused-alias-decls warnings flagged by clang-tidy v22
across 27 files in examples, exporters, sdk, and tests. The dominant
pattern was a file-scope `namespace nostd = opentelemetry::nostd;`
declaration never referenced via the file-scope alias path (uses of
`nostd::` resolved through OPENTELEMETRY_BEGIN_NAMESPACE namespace
nesting instead).

Also remove 4 unused type-alias `using M = std::map<std::string,
std::string>;` declarations in storage tests, observed by Copilot
review. clang-tidy does not flag these (misc-unused-alias-decls targets
only namespace aliases, not type aliases), but they are genuinely dead
code in the same spirit and the same files this PR already touches.

No behavior change. Removed declarations were genuinely unused.
clang-format auto-collapsed adjacent orphan alignment whitespace where
removing a line left aligned siblings.

Ratchet:
* abiv1-preview warning_limit lowered from 418 to 389
* abiv2-preview warning_limit lowered from 424 to 395

A 30th misc-unused-alias-decls site at
exporters/otlp/src/otlp_populate_attribute_utils.cc:30 is intentionally
not touched in this PR to avoid conflicting with the in-flight open-telemetry#4090.
It can be cleaned up in a trivial follow-up after open-telemetry#4090 lands.

Part of open-telemetry#2053

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
thc1006 added a commit to thc1006/opentelemetry-cpp that referenced this pull request May 18, 2026
Eliminate 29 misc-unused-alias-decls warnings flagged by clang-tidy v22
across 27 files in examples, exporters, sdk, and tests. The dominant
pattern was a file-scope `namespace nostd = opentelemetry::nostd;`
declaration never referenced via the file-scope alias path (uses of
`nostd::` resolved through OPENTELEMETRY_BEGIN_NAMESPACE namespace
nesting instead).

Also remove 4 unused type-alias `using M = std::map<std::string,
std::string>;` declarations in storage tests, observed by Copilot
review. clang-tidy does not flag these (misc-unused-alias-decls targets
only namespace aliases, not type aliases), but they are genuinely dead
code in the same spirit and the same files this PR already touches.

No behavior change. Removed declarations were genuinely unused.
clang-format auto-collapsed adjacent orphan alignment whitespace where
removing a line left aligned siblings.

Ratchet:
* abiv1-preview warning_limit lowered from 418 to 389
* abiv2-preview warning_limit lowered from 424 to 395

A 30th misc-unused-alias-decls site at
exporters/otlp/src/otlp_populate_attribute_utils.cc:30 is intentionally
not touched in this PR to avoid conflicting with the in-flight open-telemetry#4090.
It can be cleaned up in a trivial follow-up after open-telemetry#4090 lands.

Part of open-telemetry#2053

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
thc1006 added a commit to thc1006/opentelemetry-cpp that referenced this pull request May 18, 2026
Eliminate 29 misc-unused-alias-decls warnings flagged by clang-tidy v22
across 27 files in examples, exporters, sdk, and tests. The dominant
pattern was a file-scope `namespace nostd = opentelemetry::nostd;`
declaration never referenced via the file-scope alias path (uses of
`nostd::` resolved through OPENTELEMETRY_BEGIN_NAMESPACE namespace
nesting instead).

Also remove 4 unused type-alias `using M = std::map<std::string,
std::string>;` declarations in storage tests, observed by Copilot
review. clang-tidy does not flag these (misc-unused-alias-decls targets
only namespace aliases, not type aliases), but they are genuinely dead
code in the same spirit and the same files this PR already touches.

Also remove 9 now-unused #include directives that IWYU flagged as a
consequence of the alias removals: when an alias like
`namespace http_client = opentelemetry::ext::http::client;` is removed,
the include that brought the namespace into scope is no longer needed.
Same logic for several `opentelemetry/common/attribute_value.h`
includes that supported `namespace common = opentelemetry::common;`
aliases, and `<map>` after removing the unused `using M = ...` from
async_metric_storage_test.cc.

No behavior change. Removed declarations were genuinely unused.
clang-format auto-collapsed adjacent orphan alignment whitespace where
removing a line left aligned siblings.

Ratchet:
* abiv1-preview warning_limit lowered from 418 to 389
* abiv2-preview warning_limit lowered from 424 to 395

A 30th misc-unused-alias-decls site at
exporters/otlp/src/otlp_populate_attribute_utils.cc:30 is intentionally
not touched in this PR to avoid conflicting with the in-flight open-telemetry#4090.
It can be cleaned up in a trivial follow-up after open-telemetry#4090 lands.

Part of open-telemetry#2053

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
…opulate_attribute_utils

After the clang-tidy v22 upgrade in open-telemetry#4066, four narrowing-conversion
warnings surfaced in otlp_populate_attribute_utils.cc at the four
uint64_t to int64_t call sites passing to proto set_int_value.

The original code suppressed via NOLINT(cppcoreguidelines-narrowing-conversions).
In clang-tidy v22 both bugprone-narrowing-conversions and
cppcoreguidelines-narrowing-conversions are registered as separate
aliases, and NOLINT for one alias does not suppress the report under
the other. So an alias-name-rename approach cannot silence the warnings
with a single NOLINT.

Replace the four NOLINT-suppressed implicit narrowings with explicit
static_cast<int64_t> conversions, matching the pattern established in
and makes the narrowing explicit at the call site.

The fifth NOLINT at line ~297 (uint32_t to int64_t) is signed widening,
not narrowing; clang-tidy v22 emits no diagnostic there. Left untouched.

Ratchet:
* abiv1-preview warning_limit lowered from 418 to 414
* abiv2-preview warning_limit lowered from 424 to 420

Part of open-telemetry#2053

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
@thc1006 thc1006 force-pushed the codehealth/otlp-narrowing-nolint-alias branch from 2c3d074 to 785ec9a Compare May 19, 2026 16:35
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