Add noexcept on non-throwing methods identified by static analysis. by mkoscumb · Pull Request #1179 · microsoft/cpp_client_telemetry

mkoscumb · 2023-06-26T18:18:51Z

No description provided.

lalitb · 2023-08-17T03:00:30Z

@mkoscumb - Can this be merged?

lalitb · 2023-09-12T20:13:44Z

@mkoscumb - ping to merge this.

…ver-1179 # Conflicts: # lib/offline/LogSessionDataProvider.hpp # lib/system/TelemetrySystemBase.hpp # tests/functests/MultipleLogManagersTests.cpp

bmehta001 · 2026-06-12T03:05:00Z

Picking this up to get it merge-ready (the long gap left it conflicting against main).

Resolved conflicts by merging main in (not rebasing) — three files conflicted; I kept main's changes and re-applied the noexcept intent:

lib/system/TelemetrySystemBase.hpp — kept noexcept on the on*/onCleanup lambdas; took main's constructor-body closing brace.
lib/offline/LogSessionDataProvider.hpp — combined main's static with this PR's noexcept on convertStrToLong (the out-of-line definition in the .cpp already carries noexcept).
tests/functests/MultipleLogManagersTests.cpp — took main's RequestHandler(int) noexcept ctor (m_count is now a default-initialized std::atomic); noexcept preserved.

Validated on Windows (VS 2026 toolset, x64 Release): win32-lib, UnitTests, and FuncTests all build clean against current main, and the unit tests for the touched areas (LogSessionData / StringUtils / Utils / PAL / NetworkDetector) pass (26/26).

The PR is now mergeable. @lalitb's earlier approval is necessarily stale after the merge commit — a fresh review/approval is needed. Thanks @mkoscumb for the original change.

Copilot

Pull request overview

This PR adds noexcept to methods and lambdas that static analysis identified as non-throwing, tightening exception guarantees across test utilities, internal helpers, and some SDK-facing code.

Changes:

Annotated various utility and helper functions with noexcept in declarations and definitions.
Added noexcept to several small lambdas used in algorithms / predicates.
Updated session-data accessors and providers to reflect non-throwing behavior.

Reviewed changes

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

Show a summary per file

File	Description
tests/common/MockIRuntimeConfig.hpp	Marks test mock static config accessor as `noexcept`.
tests/common/Common.hpp	Marks perf-logging helpers as `noexcept`.
tests/common/Common.cpp	Updates perf-logging helper definitions to `noexcept`.
lib/utils/Utils.hpp	Marks `IsRunningInApp()` as `noexcept`.
lib/utils/Utils.cpp	Updates `IsRunningInApp()` definition to `noexcept`.
lib/utils/StringUtils.hpp	Marks several string utilities / enum-to-string helpers as `noexcept`.
lib/utils/StringUtils.cpp	Updates corresponding definitions; adds `noexcept` to small transform lambdas.
lib/utils/annex_k.hpp	Marks several static bounds-check helpers as `noexcept`.
lib/system/TelemetrySystemBase.hpp	Marks default lifecycle lambdas as `noexcept`.
lib/offline/LogSessionDataProvider.hpp	Marks constructor / accessors / parser helper as `noexcept`.
lib/offline/LogSessionDataProvider.cpp	Updates definitions for `GetLogSessionData()` / `convertStrToLong()` to `noexcept`.
lib/include/public/LogSessionData.hpp	Marks a public accessor (`getSessionFirstTime`) as `noexcept`.
lib/api/LogSessionData.cpp	Updates implementation of `getSessionFirstTime()` to `noexcept`.
lib/api/LogManagerImpl.cpp	Adds `noexcept` to several small lambdas used as predicates.

Comments suppressed due to low confidence (1)

lib/offline/LogSessionDataProvider.cpp:160

convertStrToLong checks errno after strtoll but never resets it before the call, so a previously-set errno can cause false error handling. Also, parsing a negative value will currently wrap into a large uint64_t without being rejected.

    uint64_t LogSessionDataProvider::convertStrToLong(const std::string& s) noexcept
    {
        uint64_t res = 0ull;
        char *endptr = nullptr;
        res = std::strtoll(s.c_str(), &endptr, 10);

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…negatives) Addresses the Copilot low-confidence (suppressed) review note on convertStrToLong: - strtoll's errno was checked without being reset first, so a stale errno from an earlier call could trip false "conversion failed" handling. errno is now cleared before the call. - a negative input wrapped silently into a large uint64_t. The value is now parsed into a signed temp and rejected (returns 0 + warns) if negative. - the imprecise res==LONG_MAX overflow heuristic is replaced by a direct errno==ERANGE check plus an explicit no-conversion / trailing-character check. noexcept is preserved (no throwing operations). Pre-existing logic, folded in here since this PR already annotates the same function. Verified: NDK aarch64-linux-android23 -fsyntax-only clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

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

…type Follow-up to the Copilot review on convertStrToLong: the function returns uint64_t but parsed via std::strtoll (signed long long), which constrains the accepted range to LLONG_MAX and mixes signed/unsigned. Switch to std::strtoull so parsing matches the return type and the full uint64_t range is accepted. strtoull silently wraps a leading '-', so negatives are now rejected explicitly (first non-space char check) before parsing, preserving the earlier negative-rejection behavior. noexcept preserved. Verified: NDK aarch64-linux-android23 -fsyntax-only clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

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

remove_eol only inspects and shrinks the string in place (empty(), operator[], length(), and erase() at a validated position) -- none of which allocate or throw -- so it is genuinely non-throwing. Extends this PR's noexcept coverage to the one remaining sibling helper in the session-data classes that is safely non-throwing (parse()/writeFileContents()/the std::string ctor allocate, so they correctly stay un-annotated). Verified NDK aarch64-linux-android23 -fsyntax-only. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.

EventProperties is pimpl (single EventPropertiesStorage* m_storage) with a user-declared copy ctor/assign + virtual dtor, so the compiler generated no move operations -- every pass/return/vector-growth deep-copied the whole property map via `new EventPropertiesStorage(*copy.m_storage)`. Add O(1) noexcept move ctor + move assignment that transfer the storage pointer. The dtor is already null-safe (delete nullptr); copy-assignment is now also null-safe so a moved-from object can be reassigned. Backward-compatible API addition (the single-pointer layout is unchanged). Verified: NDK aarch64-linux-android23 -fsyntax-only clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Add noexcept on non throwing methods identified by static analysis

0ac617f

mkoscumb requested review from ThomsonTan, bliptec, kindbe, lalitb, maxgolov, reyang and sid-dahiya as code owners June 26, 2023 18:18

mkoscumb added 4 commits June 26, 2023 11:19

Merge branch 'main' into user/mkoscumb/AddNoexceptToNonThrowingMethods

fcc321e

Merge branch 'main' into user/mkoscumb/AddNoexceptToNonThrowingMethods

1d9ee3e

Merge branch 'main' into user/mkoscumb/AddNoexceptToNonThrowingMethods

7e3ca2e

Merge branch 'main' into user/mkoscumb/AddNoexceptToNonThrowingMethods

a3177f2

lalitb approved these changes Jul 3, 2023

View reviewed changes

lalitb added 2 commits August 3, 2023 15:29

Merge branch 'main' into user/mkoscumb/AddNoexceptToNonThrowingMethods

3a1b79a

Merge branch 'main' into user/mkoscumb/AddNoexceptToNonThrowingMethods

3cab212

Merge remote-tracking branch 'msft/main' into bhamehta/noexcept-takeo…

d099b1f

…ver-1179 # Conflicts: # lib/offline/LogSessionDataProvider.hpp # lib/system/TelemetrySystemBase.hpp # tests/functests/MultipleLogManagersTests.cpp

bmehta001 requested a review from a team as a code owner June 12, 2026 03:03

bmehta001 requested review from Copilot and removed request for bliptec and kindbe June 12, 2026 03:05

Copilot started reviewing on behalf of bmehta001 June 12, 2026 03:05 View session

Copilot AI reviewed Jun 12, 2026

View reviewed changes

Comment thread lib/include/public/LogSessionData.hpp

bmehta001 and others added 2 commits June 16, 2026 20:15

Merge branch 'main' into user/mkoscumb/AddNoexceptToNonThrowingMethods

225ee96

bmehta001 requested a review from Copilot June 17, 2026 01:19

Copilot started reviewing on behalf of bmehta001 June 17, 2026 01:19 View session

Copilot AI reviewed Jun 17, 2026

View reviewed changes

Comment thread lib/offline/LogSessionDataProvider.cpp

bmehta001 requested a review from Copilot June 18, 2026 06:25

Copilot started reviewing on behalf of bmehta001 June 18, 2026 06:26 View session

Copilot AI reviewed Jun 18, 2026

View reviewed changes

Comment thread lib/offline/LogSessionDataProvider.hpp

Comment thread lib/offline/LogSessionDataProvider.cpp

Comment thread lib/offline/LogSessionDataProvider.cpp

bmehta001 requested a review from Copilot June 18, 2026 07:02

Copilot started reviewing on behalf of bmehta001 June 18, 2026 07:03 View session

Copilot AI reviewed Jun 18, 2026

View reviewed changes

bmehta001 and others added 2 commits June 18, 2026 17:40

Merge branch 'main' into user/mkoscumb/AddNoexceptToNonThrowingMethods

e5419e3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add noexcept on non-throwing methods identified by static analysis.#1179

Add noexcept on non-throwing methods identified by static analysis.#1179
mkoscumb wants to merge 14 commits into
mainfrom
user/mkoscumb/AddNoexceptToNonThrowingMethods

mkoscumb commented Jun 26, 2023

Uh oh!

lalitb commented Aug 17, 2023

Uh oh!

lalitb commented Sep 12, 2023

Uh oh!

bmehta001 commented Jun 12, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

mkoscumb commented Jun 26, 2023

Uh oh!

lalitb commented Aug 17, 2023

Uh oh!

lalitb commented Sep 12, 2023

Uh oh!

bmehta001 commented Jun 12, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants