Bhamehta/code cleanup - #2
Open
bmehta001 wants to merge 26 commits into
Open
Conversation
bmehta001
force-pushed
the
bhamehta/code-cleanup
branch
20 times, most recently
from
March 19, 2026 05:40
9a5a166 to
2feeb94
Compare
bmehta001
force-pushed
the
bhamehta/code-cleanup
branch
4 times, most recently
from
April 1, 2026 16:55
e303041 to
9a91839
Compare
bmehta001
force-pushed
the
bhamehta/code-cleanup
branch
5 times, most recently
from
April 29, 2026 21:29
b30c9b8 to
95502d5
Compare
bmehta001
force-pushed
the
bhamehta/code-cleanup
branch
from
May 15, 2026 03:06
b34147f to
0e8018c
Compare
bmehta001
force-pushed
the
bhamehta/code-cleanup
branch
from
May 22, 2026 07:35
17b72ca to
1d09c10
Compare
- Modernize CMakeLists.txt: flatten nesting, deduplicate, use consistent quoting and variable patterns - Remove stale header references from vcxproj and vcxitems files - Simplify test CMakeLists.txt files - Fix CMake conventions in packaging scripts Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Keep the before.targets toolset selection deterministic on newer Visual Studio hosts, but only set the MIP props fallback when a consumer has not already chosen a toolset. While addressing the MSBuild review comments, also point the optional module test conditions and source paths at the real lib/modules locations and use CPACK_PACKAGE_FILE_NAME for the RPM status message. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Quote the optional ECS test.json configure_file input and output paths so CMake handles source or build directories that contain spaces. Files changed: - tests/functests/CMakeLists.txt Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
PR microsoft#1416 inadvertently regressed PR microsoft#1415 by removing -fno-finite-math-only from the three Unix REL_FLAGS branches and adding -Wno-nan-infinity-disabled to silence the Clang diagnostic. That left release builds using -ffast-math without preserving the NaN/Inf semantics needed by nlohmann::json and SQLite paths. Remove -ffast-math entirely from the GCC, AppleClang, and generic Clang release flags rather than relying on -fno-finite-math-only to partially undo it. This SDK is not floating-point compute-bound; its hot paths are string, Bond/JSON serialization, HTTP I/O, and SQLite reads/writes. Avoiding -ffast-math: - preserves std::isnan/std::isinf behavior for JSON and storage code, - avoids compiler/runtime fast-math side effects such as x86 GCC's crtfastmath.o changing MXCSR FTZ/DAZ behavior process-wide, and - aligns with SQLite's guidance to avoid fast-math. This subsumes microsoft#1415's partial mitigation and aligns with microsoft#1392's intent, extended to GCC because GCC fast-math has the same broad assumptions and runtime side effects. Also remove -Wno-nan-infinity-disabled because the warning should not be suppressed once the cause is gone. Validation: - CMake Release build of UnitTests on macOS arm64 (AppleClang 21). - UnitTests passed on macOS arm64. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Avoid carrying a confusing note about warning flags that are not used, and keep CMakeLists.txt focused on active compiler settings. Files changed: - CMakeLists.txt Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
bmehta001
force-pushed
the
bhamehta/code-cleanup
branch
from
May 22, 2026 20:09
1d09c10 to
1cfe326
Compare
Copilot review on PR microsoft#1416 flagged 7 unquoted ${CMAKE_SOURCE_DIR}/... paths inside list(APPEND SRCS ...) calls. CMake splits unquoted arguments on whitespace, so a checkout path containing spaces (very common on Windows: C:\Users\First Last\source\...) would fragment a single source path into multiple list elements and the test wouldn't compile. Quote every ${CMAKE_SOURCE_DIR}/.../*.cpp path inside list(APPEND SRCS ...) in both tests/unittests/CMakeLists.txt and tests/functests/CMakeLists.txt (23 paths total, including the privacyguard + sanitizer blocks Copilot didn't cite but that share the same hazard). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot review on PR microsoft#1416 flagged that mapping VisualStudioVersion >= 18.0 to PlatformToolset v145 is a guess: VS 2025/2026/etc. has not shipped, the toolset moniker for that release may differ, and pinning to a non-existent toolset name causes MSBuild to fail early with "unknown PlatformToolset" on any machine that does pick up that VS version. Drop the v145 mapping from both before.targets and build.MIP.props. Future VS versions fall through to the v141 fallback (same behavior as before PR microsoft#1416, just one less wrong-toolset-name failure mode). Add explicit mappings for new toolsets as they actually ship. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The top-level CMakeLists.txt asks for C++14 (CMAKE_CXX_STANDARD 14, set in 2018) but then re-injects -std=c++11 into CMAKE_CXX_FLAGS for both Release and Debug Unix builds (set in 2019 when gcc-4.x / gcc-5.x C++14 support was still spotty). That explicit flag wins, so the Unix builds have actually been C++11 ever since, contradicting the stated standard. This is a vestige: gcc-5+, clang-3.4+, and MSVC 2015+ all support C++14 fully, and Android (lib/android_build/) already sets CMAKE_CXX_STANDARD 14 explicitly. Removing the override aligns the Unix CMake builds with the intended standard. - Drop the stale gcc-4.x/5.x caveat comment and add CMAKE_CXX_STANDARD_REQUIRED ON so the build fails loudly on toolchains that cannot do C++14, rather than silently falling back. - Drop the -std=c++11 fragment from both REL and DBG CMAKE_CXX_FLAGS lines. C and C11 are left alone (C11 remains the C floor). Not touched in this commit: - examples/**/CMakeLists.txt and wrappers/obj-c/CMakeLists.txt also pin -std=c++11. Those are standalone sample/wrapper build scripts not wired via add_subdirectory and not built by any CI workflow; their pins are intentional statements of each sample's minimum and can stay. - This commit does not bump the project to C++17. Cleanup ideas like cpp_client_telemetry_modules#302 (std::string::data() -> char*) require C++17 and are still out of scope. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Derive package version components from MATSDK_BUILD_VERSION so CPack packages stay aligned with the SDK version. Use PROJECT_SOURCE_DIR and PROJECT_BINARY_DIR for test CMake paths so the SDK remains subproject-friendly. Files changed: - CMakeLists.txt - tools/MakeDeb.cmake - tools/MakeRpm.cmake - tools/MakeTgz.cmake - tests/functests/CMakeLists.txt - tests/unittests/CMakeLists.txt Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Validate MATSDK_BUILD_VERSION before deriving CPack version components and remove now-dead DAYNUMBER calculations from package scripts. Files changed: - CMakeLists.txt - tools/MakeDeb.cmake - tools/MakeRpm.cmake - tools/MakeTgz.cmake Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
# Conflicts: # lib/CMakeLists.txt
The previous merge from main left a conflict marker block in lib/CMakeLists.txt's trailing commented-link section. Remove the marker block and keep the cleaned-up file content. Files changed: - lib/CMakeLists.txt Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Keep package script version cleanup out of the CMake cleanup PR; package version alignment can be handled separately before the next release. Files changed: - CMakeLists.txt - tools/MakeDeb.cmake - tools/MakeRpm.cmake - tools/MakeTgz.cmake Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add an explicit VS2026/v145 command-line build entry alongside the existing VS2022/v143 script so Windows consumers can choose the right supported toolset without guessing from MSBuild internals. Files changed: - docs/cpp-start-windows.md - build-all-v145.bat - tools/vcvars.cmd - Solutions/before.targets Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This reverts commit b99ff14.
zlib/CMakeLists.txt still targets old CMake compatibility, but add_compile_definitions requires newer CMake. Restore add_definitions for the Windows preprocessor definitions so the zlib subproject remains compatible with the branch's stated CMake floor. Validation: - cmake configure with CMAKE_POLICY_VERSION_MINIMUM=3.5 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes runtime thread-safety, shutdown-safety, and response-lifetime issues that affect normal SDK operation. Split out from microsoft#1415 per reviewer request so runtime behavior changes stay separate from CI/build/test fixes.
Fix HTTP cancellation and callback tracking
HttpClient_Apple.mm
m_dataTaskinstead of canceling every task on the shared staticNSURLSession.CancelAllRequests()wait loop som_requests.empty()is checked while holdingm_requestsMtx.HttpClientManager.cpp
cancelAllRequests()wait loop som_httpCallbacks.empty()is checked while holdingm_httpCallbacksMtx.Fix HTTP response lifetime on abort/network-failure paths
HttpResponseDecoder.cpp
ctx->httpResponsethroughrequestAborted(ctx)andtemporaryNetworkFailure(ctx)so downstream storage/statistics handlers can still read status and headers.EventsUploadContext, whoseclear()path deletes the response.HttpResponseDecoderTests.cpp
Fix WorkerThread shutdown safety
WorkerThread.cpp
Queue()calls are rejected once teardown starts.join(), and avoid cleanup afterdetach()because the worker may still be running.Make TransmissionPolicyManager scheduling consistently mutex-guarded
TransmissionPolicyManager.cpp / .hpp
m_isUploadScheduled,m_runningLatency, andm_scheduledUploadTimeconsistently withm_scheduledUploadMutex.m_scheduledUploadMutexacross potentially blocking cancellation during stop/shutdown.std::chrono::millisecondsvalue in the bandwidth-controller reschedule path.TransmissionPolicyManagerTests.cpp
Fix Logger static-destruction-order crash
Logger.cpp
Logger::~Logger()because it can run after logging infrastructure has already been destroyed, causing crashes during static teardown.Validation
HttpResponseDecoderTests.*andTransmissionPolicyManagerTests.ForceScheduleRetainsImmediateUploadWhenCancelBlockspassed.UnitTestspassed:485/485