Problem
include/reflection_export.h disables a set of MSVC warnings at the top of the file, including C4996 (use of a deprecated declaration):
#pragma warning(disable : 4018) // signed/unsigned warnings
#pragma warning(disable : 4100)
#pragma warning(disable : 4251)
#pragma warning(disable : 4512)
#pragma warning(disable : 4996)
C4996 is the warning that would have flagged std::wstring_convert and <codecvt>, the subject of #25. Because it was suppressed, a facet deprecated since C++17 sat in the conversion path without a single build warning on any of the four MSVC CI legs. #25 was found by reasoning about wchar_t widths, not by the toolchain telling anyone.
<codecvt> is gone now, so nothing in the library currently depends on the suppression for that reason — but nothing would flag a reintroduction either, whether of <codecvt> or of any other deprecated API.
Two separate concerns
1. The suppression hides real deprecation warnings. This is the one that bit us in #25.
2. It is a public header. reflection_export.h is under include/ and is pulled in by the public API headers, so every one of these #pragma warning(disable: ...) lines leaks into the translation unit of anyone consuming the library. A consumer who includes database.h silently loses C4996 — and C4018, C4100, C4251, C4512 — in their own code. A library should not turn off its users' warnings as a side effect of being included.
Suggested direction
- Establish whether the library still needs C4996 suppressed at all. If some vendored source needs it (
src/sqlite3.c is the likely candidate), scope it to that file with push/pop rather than a public header.
- Same question for the other four codes, which have no explanatory comment beyond C4018's.
- If any must stay, move them behind
BUILD_SQLITE_REFLECTION so they apply while building the library but not to consumers, and pair each with a comment saying what needs it.
- Worth confirming afterwards that a deliberate use of a deprecated API does produce a warning on the MSVC CI legs, so the guard is verified rather than assumed.
Notes
Pre-existing, and out of scope for #40 — CONTRIBUTING.md asks that lint fixes stay scoped to the code a change touches. Raised during review of that PR.
Problem
include/reflection_export.hdisables a set of MSVC warnings at the top of the file, including C4996 (use of a deprecated declaration):C4996 is the warning that would have flagged
std::wstring_convertand<codecvt>, the subject of #25. Because it was suppressed, a facet deprecated since C++17 sat in the conversion path without a single build warning on any of the four MSVC CI legs. #25 was found by reasoning aboutwchar_twidths, not by the toolchain telling anyone.<codecvt>is gone now, so nothing in the library currently depends on the suppression for that reason — but nothing would flag a reintroduction either, whether of<codecvt>or of any other deprecated API.Two separate concerns
1. The suppression hides real deprecation warnings. This is the one that bit us in #25.
2. It is a public header.
reflection_export.his underinclude/and is pulled in by the public API headers, so every one of these#pragma warning(disable: ...)lines leaks into the translation unit of anyone consuming the library. A consumer who includesdatabase.hsilently loses C4996 — and C4018, C4100, C4251, C4512 — in their own code. A library should not turn off its users' warnings as a side effect of being included.Suggested direction
src/sqlite3.cis the likely candidate), scope it to that file withpush/poprather than a public header.BUILD_SQLITE_REFLECTIONso they apply while building the library but not to consumers, and pair each with a comment saying what needs it.Notes
Pre-existing, and out of scope for #40 — CONTRIBUTING.md asks that lint fixes stay scoped to the code a change touches. Raised during review of that PR.