Skip to content

FIX: Fixing Level 3 and level 4 compiler warnings. #72

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

gargsaumya
Copy link
Contributor

@gargsaumya gargsaumya commented Jun 10, 2025

AB#37476
This pull request focuses on improving type safety, enhancing error handling, and adding utility functions in the mssql_python/pybind module. The changes primarily involve the use of explicit type casting, the introduction of a WideToUTF8 utility function for string conversion, and minor fixes to improve code clarity and correctness.

Type Safety Improvements:

  • Updated std::make_shared<SqlHandle> calls across multiple functions in connection.cpp to use static_cast<SQLSMALLINT> for handle types (SQL_HANDLE_ENV, SQL_HANDLE_DBC, SQL_HANDLE_STMT) to ensure explicit type casting. [1] [2] [3]
  • Added explicit type casting for SQL date, time, and timestamp fields in BindParameters to use SQLSMALLINT or SQLUSMALLINT instead of int. [1] [2] [3]
  • Updated various SQLSetStmtAttr_ptr and SQLSetConnectAttr_ptr calls to use reinterpret_cast or static_cast for pointer conversions, ensuring type correctness. [1] [2] [3]

Error Handling Enhancements:

  • Replaced manual wide-to-narrow string conversion in LoadDriverOrThrowException with the new WideToUTF8 utility function for better readability and consistency.
  • Introduced the WideToUTF8 function in ddbc_bindings.cpp and declared it in ddbc_bindings.h to handle std::wstring to std::string conversions. [1] [2]
  • Enhanced error message handling in Connection::checkError by using WideToUTF8 for converting wide error messages to UTF-8.

Code Fixes and Cleanup:

  • Fixed a typo in std::memset in BindParameters to correct the namespace usage.
  • Corrected redundant semicolon in SQLGetData_wrap initialization.
  • Improved comparison logic in SQLGetData_wrap and FetchBatchData to use static_cast<size_t> for comparing dataLen with columnSize. [1] [2]

These changes collectively enhance the robustness, maintainability, and readability of the code.

Checklist

  • Tests Passed (if applicable) : All pytests passed.

@Copilot Copilot AI review requested due to automatic review settings June 10, 2025 03:39
Copy link

@Copilot 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

This PR addresses compiler warnings by adding explicit type casts for ODBC handles and parameters, introduces a utility for wide-to-UTF8 string conversion, and applies small cleanups for clarity.

  • Added WideToUTF8 to centralize wstring→UTF-8 conversion and updated error/log messages to use it.
  • Applied static_cast/reinterpret_cast across handle creation and parameter binding for strict type safety.
  • Fixed minor typos (e.g., std::memset, redundant semicolons) and improved length comparisons.

Reviewed Changes

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

File Description
mssql_python/pybind/ddbc_bindings.h Declared new WideToUTF8 utility function
mssql_python/pybind/ddbc_bindings.cpp Implemented WideToUTF8, casts in BindParameters, cleanup
mssql_python/pybind/connection/connection.cpp Casted handle types, used WideToUTF8, removed unused var
Comments suppressed due to low confidence (3)

mssql_python/pybind/ddbc_bindings.h:181

  • [nitpick] Consider placing WideToUTF8 inside a dedicated namespace (e.g., ddbc) to avoid global namespace pollution.
std::string WideToUTF8(const std::wstring& wstr);

mssql_python/pybind/ddbc_bindings.cpp:475

  • Add unit tests for WideToUTF8 to verify conversion results and edge cases like empty or invalid wide strings.
std::string WideToUTF8(const std::wstring& wstr) {

mssql_python/pybind/connection/connection.cpp:125

  • [nitpick] Add a brief comment explaining the cast chain (static_cast then reinterpret_cast) for SQLSetConnectAttr to clarify why both are needed.
SQLRETURN ret = SQLSetConnectAttr_ptr(_dbcHandle->get(), SQL_ATTR_AUTOCOMMIT, reinterpret_cast<SQLPOINTER>(static_cast<SQLULEN>(value)), 0);

@gargsaumya gargsaumya force-pushed the saumya/resolve-warnings branch from f772912 to eb3b5db Compare June 10, 2025 03:48
@gargsaumya gargsaumya requested a review from sumitmsft June 10, 2025 03:50
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.

1 participant