fixes of zip extraction and xml parsing & tests

Author: nick

.gitignore

@@ -4,4 +4,9 @@
 
 /.vs
 /.venv
-BUILDING.md
+BUILDING.md
+/.vscode
+/build
+test.py
+report.docx
+docx_comment_parser.pyd

CMakeLists.txt

@@ -1,68 +1,114 @@
 cmake_minimum_required(VERSION 3.15)
 project(docx_comment_parser VERSION 1.0.0 LANGUAGES CXX)
 
-# ─── Standard & optimisation flags ──────────────────────────────────────────
-set(CMAKE_CXX_STANDARD 17)
+# ─── Policy: normalise install() DESTINATION paths (fixes CMP0177 warning) ───
+# Python3_SITEARCH on MinGW contains backslashes which CMake 3.26+ normalises
+# automatically under the NEW policy.  Setting it explicitly silences the warning
+# on all CMake versions ≥ 3.15 that support CMP0177.
+if(POLICY CMP0177)
+  cmake_policy(SET CMP0177 NEW)
+endif()
+
+# ─── C++ standard ─────────────────────────────────────────────────────────────
+set(CMAKE_CXX_STANDARD          17)
 set(CMAKE_CXX_STANDARD_REQUIRED ON)
-set(CMAKE_CXX_EXTENSIONS OFF)
+set(CMAKE_CXX_EXTENSIONS        OFF)
 
 if(NOT CMAKE_BUILD_TYPE)
   set(CMAKE_BUILD_TYPE Release)
 endif()
 
-# LTO for Release builds
-include(CheckIPOSupported)
-check_ipo_supported(RESULT _ipo_ok OUTPUT _ipo_err)
-if(_ipo_ok)
-  set(CMAKE_INTERPROCEDURAL_OPTIMIZATION_RELEASE ON)
+# ─── LTO (Release only, skip on MinGW where it is unreliable) ─────────────────
+if(NOT MINGW)
+  include(CheckIPOSupported)
+  check_ipo_supported(RESULT _ipo_ok OUTPUT _ipo_err)
+  if(_ipo_ok)
+    set(CMAKE_INTERPROCEDURAL_OPTIMIZATION_RELEASE ON)
+  endif()
 endif()
 
-# ─── Dependencies ────────────────────────────────────────────────────────────
-# libxml2 not required - using built-in XML parser
-find_package(ZLIB    REQUIRED)
+# ─── Dependencies ─────────────────────────────────────────────────────────────
+# No libxml2 required — self-contained XML parser.
+#
+# zlib strategy:
+#   MSVC   — no system zlib exists.  zip_reader.cpp compiles the vendored
+#             single-header inflate from vendor/zlib/zlib.h; no link step needed.
+#   MinGW  — zlib1.dll + libz.a ship with every MinGW-w64 installation.
+#   Linux / macOS — system zlib (apt install zlib1g-dev / brew install zlib).
+if(NOT MSVC)
+  find_package(ZLIB REQUIRED)
+endif()
 
-# ─── Core shared library ─────────────────────────────────────────────────────
+# ─── Core shared library ──────────────────────────────────────────────────────
 add_library(docx_comment_parser SHARED
     src/docx_parser.cpp
     src/batch_parser.cpp
     src/zip_reader.cpp
     src/xml_parser.cpp
 )
 
+# Tell the compiler which TUs are *building* the DLL so DOCX_API expands to
+# dllexport; consumers that only include the header get dllimport instead.
+target_compile_definitions(docx_comment_parser PRIVATE DOCX_BUILDING_DLL)
+
 target_include_directories(docx_comment_parser
     PUBLIC
         $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>
         $<INSTALL_INTERFACE:include>
     PRIVATE
+        # On MSVC the vendored zlib.h is included by zip_reader.cpp via a
+        # relative path "../vendor/zlib/zlib.h".  Adding vendor/ here makes
+        # the path work regardless of which directory cl.exe is invoked from.
+        $<$<CXX_COMPILER_ID:MSVC>:${CMAKE_CURRENT_SOURCE_DIR}/vendor>
 )
 
 target_link_libraries(docx_comment_parser
     PRIVATE
-        ZLIB::ZLIB
+        # Link system zlib on non-MSVC platforms only.
+        # On MSVC, inflate is compiled directly into zip_reader.cpp.
+        $<$<NOT:$<CXX_COMPILER_ID:MSVC>>:ZLIB::ZLIB>
 )
 
-# Hide all symbols except those explicitly exported with DOCX_API
-set_target_properties(docx_comment_parser PROPERTIES
-    CXX_VISIBILITY_PRESET hidden
-    VISIBILITY_INLINES_HIDDEN ON
-    VERSION   ${PROJECT_VERSION}
-    SOVERSION 1
-)
+# On MinGW, link the standard threading and networking libs that std::thread
+# and socket code pull in indirectly.
+if(MINGW)
+  target_link_libraries(docx_comment_parser PRIVATE -lws2_32 -lmswsock)
+endif()
+
+# Symbol visibility — ELF only.  On Windows (PE/DLL) visibility is handled by
+# __declspec(dllexport/dllimport) in the header; the GCC attribute is a no-op
+# there but can confuse some linker versions, so guard it explicitly.
+if(NOT WIN32)
+  set_target_properties(docx_comment_parser PROPERTIES
+      CXX_VISIBILITY_PRESET  hidden
+      VISIBILITY_INLINES_HIDDEN ON
+  )
+endif()
+
+# VERSION / SOVERSION are ELF-only concepts (produce .so.1 symlinks on Linux).
+# MinGW/Windows uses a different DLL versioning mechanism; setting these
+# properties on a PE target causes the ld "error: ld returned 5" link failure.
+if(NOT WIN32)
+  set_target_properties(docx_comment_parser PROPERTIES
+      VERSION   ${PROJECT_VERSION}
+      SOVERSION 1
+  )
+endif()
 
 target_compile_options(docx_comment_parser PRIVATE
     $<$<CXX_COMPILER_ID:GNU,Clang>:-Wall -Wextra -Wpedantic>
     $<$<CONFIG:Release>:-O3 -DNDEBUG>
 )
 
-# ─── Python extension (optional) ─────────────────────────────────────────────
+# ─── Python extension (optional) ──────────────────────────────────────────────
 option(BUILD_PYTHON_BINDINGS "Build Python bindings via pybind11" ON)
 
 if(BUILD_PYTHON_BINDINGS)
     find_package(Python3 REQUIRED COMPONENTS Interpreter Development)
     find_package(pybind11 CONFIG QUIET)
 
     if(NOT pybind11_FOUND)
-        # Try to locate pybind11 via pip-installed package
+        # Locate pybind11 installed via pip
         execute_process(
             COMMAND ${Python3_EXECUTABLE} -c "import pybind11; print(pybind11.get_cmake_dir())"
             OUTPUT_VARIABLE _pybind11_cmake_dir
@@ -79,48 +125,55 @@ if(BUILD_PYTHON_BINDINGS)
             python/python_bindings.cpp
         )
 
+        # The extension builds *into* the DLL — it also needs DOCX_BUILDING_DLL
+        # so that DOCX_API expands to dllexport when compiling its TU on Windows.
+        target_compile_definitions(_docx_comment_parser PRIVATE DOCX_BUILDING_DLL)
+
         target_include_directories(_docx_comment_parser PRIVATE
             ${CMAKE_CURRENT_SOURCE_DIR}/include
-            
+            # Vendor dir for MSVC (same reason as the main library target)
+            $<$<CXX_COMPILER_ID:MSVC>:${CMAKE_CURRENT_SOURCE_DIR}/vendor>
         )
 
         target_link_libraries(_docx_comment_parser PRIVATE
             docx_comment_parser
-            
-            ZLIB::ZLIB
+            $<$<NOT:$<CXX_COMPILER_ID:MSVC>>:ZLIB::ZLIB>
         )
 
-        # Install alongside the Python package
+        # CMP0177 NEW: CMake normalises the path before passing it to install(),
+        # so backslashes from Python3_SITEARCH on Windows are converted to slashes.
         install(TARGETS _docx_comment_parser
-            LIBRARY DESTINATION ${Python3_SITEARCH}
+            LIBRARY DESTINATION "${Python3_SITEARCH}"
+            RUNTIME DESTINATION "${Python3_SITEARCH}"
         )
     else()
-        message(WARNING "pybind11 not found – Python bindings will not be built. "
-                        "Install with: pip install pybind11")
+        message(WARNING
+            "pybind11 not found — Python bindings will not be built.\n"
+            "Install with: pip install pybind11")
     endif()
 endif()
 
 # ─── Install rules ────────────────────────────────────────────────────────────
 include(GNUInstallDirs)
 
 install(TARGETS docx_comment_parser
-    EXPORT  docx_comment_parserTargets
-    LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
-    ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}
-    RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}
-    INCLUDES DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}
+    EXPORT   docx_comment_parserTargets
+    LIBRARY  DESTINATION "${CMAKE_INSTALL_LIBDIR}"
+    ARCHIVE  DESTINATION "${CMAKE_INSTALL_LIBDIR}"
+    RUNTIME  DESTINATION "${CMAKE_INSTALL_BINDIR}"
+    INCLUDES DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}"
 )
 
 install(FILES
     include/docx_comment_parser.h
     include/zip_reader.h
-    DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/docx_comment_parser
+    DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}/docx_comment_parser"
 )
 
 install(EXPORT docx_comment_parserTargets
-    FILE        docx_comment_parserTargets.cmake
-    NAMESPACE   docx::
-    DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/docx_comment_parser
+    FILE      docx_comment_parserTargets.cmake
+    NAMESPACE docx::
+    DESTINATION "${CMAKE_INSTALL_LIBDIR}/cmake/docx_comment_parser"
 )
 
 # ─── Tests ────────────────────────────────────────────────────────────────────

include/docx_comment_parser.h

@@ -1,7 +1,24 @@
 #pragma once
 
+// ─── Symbol export/import macro ───────────────────────────────────────────────
+//
+// On Windows (MSVC and MinGW) DLL symbols must be explicitly exported by the
+// *building* translation unit and imported by *consuming* translation units.
+//
+//   DOCX_BUILDING_DLL  – defined by CMake / setup.py when compiling the library
+//                        itself; expands DOCX_API to __declspec(dllexport).
+//                        NOT defined when a downstream project includes this header,
+//                        so DOCX_API expands to __declspec(dllimport) instead.
+//
+// On Linux/macOS with -fvisibility=hidden the macro marks symbols as default
+// visibility so the linker exports them even though the TU default is hidden.
+
 #ifdef _WIN32
-  #define DOCX_API __declspec(dllexport)
+  #ifdef DOCX_BUILDING_DLL
+    #define DOCX_API __declspec(dllexport)
+  #else
+    #define DOCX_API __declspec(dllimport)
+  #endif
 #else
   #define DOCX_API __attribute__((visibility("default")))
 #endif
@@ -43,12 +60,12 @@ struct DOCX_API CommentMetadata {
 
     // --- content ---
     std::string text;                    // full plain-text of comment body
-    std::string paragraph_style;        // style of first paragraph inside comment
+    std::string paragraph_style;         // style of first paragraph inside comment
 
     // --- anchoring ---
-    std::string range_start_para_id;    // w:commentRangeStart / paraId attribute (OOXML 2016+)
+    std::string range_start_para_id;     // w:commentRangeStart / paraId (OOXML 2016+)
     std::string range_end_para_id;
-    std::string referenced_text;        // the document text the comment is anchored to
+    std::string referenced_text;         // document text the comment is anchored to
 
     // --- threading (OOXML 2016+) ---
     bool        is_reply{false};
@@ -61,8 +78,8 @@ struct DOCX_API CommentMetadata {
     bool        done{false};             // @w16cex:done
 
     // --- document position ---
-    int         paragraph_index{-1};    // 0-based paragraph in document body
-    int         run_index{-1};          // 0-based run within that paragraph
+    int         paragraph_index{-1};     // 0-based paragraph in document body
+    int         run_index{-1};           // 0-based run within that paragraph
 
     // Convenience: resolved full thread chain (only set on root comments)
     std::vector<int> thread_ids;         // ordered list of ids in this thread
@@ -90,8 +107,8 @@ struct DOCX_API DocxParserError : std::runtime_error {
     explicit DocxParserError(const std::string& msg) : std::runtime_error(msg) {}
 };
 
-struct DOCX_API DocxFileError   : DocxParserError {
-    explicit DocxFileError(const std::string& msg)   : DocxParserError(msg) {}
+struct DOCX_API DocxFileError : DocxParserError {
+    explicit DocxFileError(const std::string& msg) : DocxParserError(msg) {}
 };
 
 struct DOCX_API DocxFormatError : DocxParserError {

setup.py

@@ -7,11 +7,31 @@
 
 Or build in-place:
     python setup.py build_ext --inplace
+
+Supported toolchains:
+    Linux / macOS  : GCC or Clang          (uses -std=c++17, -lz)
+    Windows MSVC   : Visual Studio 2019+   (uses /std:c++17)
+    Windows MinGW  : MinGW-w64 via MSYS2   (uses -std=c++17, -lz)
 """
 
 from setuptools import setup, Extension
 import sys
 import os
+import subprocess
+
+
+def _is_mingw() -> bool:
+    """Return True when building with MinGW-w64 GCC under Windows."""
+    if sys.platform != "win32":
+        return False
+    try:
+        out = subprocess.check_output(
+            ["gcc", "--version"], stderr=subprocess.DEVNULL
+        ).decode()
+        return "mingw" in out.lower() or "MINGW" in out
+    except (FileNotFoundError, subprocess.CalledProcessError):
+        return False
+
 
 # ─── Locate pybind11 headers ──────────────────────────────────────────────────
 try:
@@ -20,16 +40,43 @@
 except ImportError:
     raise RuntimeError("pybind11 not found. Install with: pip install pybind11")
 
-# ─── Platform-specific flags ─────────────────────────────────────────────────
-if sys.platform == "win32":
-    extra_compile_args = ["/std:c++17", "/O2", "/DNDEBUG", "/EHsc"]
-    extra_link_args    = []
+# ─── Platform / toolchain flags ──────────────────────────────────────────────
+IS_MINGW = _is_mingw()
+IS_MSVC  = sys.platform == "win32" and not IS_MINGW
+
+if IS_MSVC:
+    # MSVC (cl.exe) — Visual Studio 2019 or later
+    extra_compile_args = [
+        "/std:c++17",
+        "/O2",
+        "/DNDEBUG",
+        "/EHsc",                    # enable C++ exception handling
+        "/DDOCX_BUILDING_DLL",      # expand DOCX_API to __declspec(dllexport)
+    ]
+    extra_link_args = []            # zlib linked via vcpkg / find_package
+
+elif IS_MINGW:
+    # MinGW-w64 GCC on Windows (MSYS2 / standalone)
+    extra_compile_args = [
+        "-std=c++17",
+        "-O2",
+        "-DNDEBUG",
+        "-DDOCX_BUILDING_DLL",      # expand DOCX_API to __declspec(dllexport)
+        "-Wall",
+    ]
+    extra_link_args = [
+        "-lz",                      # zlib1.dll — ships with every MinGW installation
+        "-lws2_32",                 # Winsock — required by std::thread on MinGW
+        "-lmswsock",
+    ]
+
 else:
+    # Linux / macOS — GCC or Clang
     extra_compile_args = [
         "-std=c++17",
         "-O3",
         "-DNDEBUG",
-        "-fvisibility=hidden",
+        "-fvisibility=hidden",      # hide all symbols except those marked DOCX_API
     ]
     extra_link_args = ["-lz"]
 
@@ -45,7 +92,7 @@
 ext = Extension(
     "docx_comment_parser",
     sources=sources,
-    include_dirs=["include", pybind11_include],
+    include_dirs=["include", "vendor", pybind11_include],
     extra_compile_args=extra_compile_args,
     extra_link_args=extra_link_args,
     language="c++",

src/xml_parser.cpp

@@ -148,8 +148,10 @@ bool sax_parse(const char* data, std::size_t len, SaxHandler& h) {
 
         // Processing instruction or XML declaration
         if (*p == '?') {
-            while (p < end && *p != '>') ++p;
-            if (p < end) ++p;
+            ++p;
+            // Scan for the closing "?>" sequence, not just ">"
+            while (p + 1 < end && !(p[0] == '?' && p[1] == '>')) ++p;
+            if (p + 1 < end) p += 2;  // skip "?>"
             continue;
         }