Skip to content

Commit 8383f49

Browse files
committed
Final polish
* Run emscripten tests * Very minor BasicCSVParser constructor fix * Documentation updates
1 parent 8b702ef commit 8383f49

4 files changed

Lines changed: 33 additions & 12 deletions

File tree

.github/workflows/cmake-multi-platform.yml

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -146,11 +146,16 @@ jobs:
146146
-DCSV_ENABLE_THREADS=OFF
147147
-DCSV_CXX_STANDARD=20
148148
-DCSV_BUILD_SINGLE_INCLUDE_TEST=ON
149+
-DCMAKE_CROSSCOMPILING_EMULATOR=node
149150
-DCMAKE_BUILD_TYPE=Release
150151
-S ${{ github.workspace }}
151152
152153
- name: Build (single-threaded emscripten)
153154
run: cmake --build ${{ github.workspace }}/build-single-thread-emscripten --config Release
154155

156+
- name: Test (single-threaded emscripten)
157+
working-directory: ${{ github.workspace }}/build-single-thread-emscripten
158+
run: ctest --build-config Release --output-on-failure
159+
155160

156161

include/internal/basic_csv_parser.cpp

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,19 @@ namespace csv {
66
namespace internals {
77
CSV_INLINE size_t get_file_size(csv::string_view filename) {
88
std::ifstream infile(std::string(filename), std::ios::binary);
9+
if (!infile.is_open()) {
10+
throw std::runtime_error("Cannot open file " + std::string(filename));
11+
}
12+
913
const auto start = infile.tellg();
1014
infile.seekg(0, std::ios::end);
1115
const auto end = infile.tellg();
1216

13-
return end - start;
17+
if (start < 0 || end < 0) {
18+
throw std::runtime_error("Cannot determine file size for " + std::string(filename));
19+
}
20+
21+
return static_cast<size_t>(end - start);
1422
}
1523

1624
CSV_INLINE std::string get_csv_head(csv::string_view filename) {

include/internal/csv_reader.hpp

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -159,25 +159,26 @@ namespace csv {
159159
* Constructors for iterating over large files and parsing in-memory sources.
160160
*/
161161
///@{
162-
/** @brief Construct CSVReader from filename using memory-mapped I/O
162+
/** @brief Construct CSVReader from filename.
163163
*
164-
* CODE PATH 1 of 2: Uses MmapParser with mio library for maximum performance.
165-
* This is fundamentally different from the stream-based constructor below.
164+
* Native builds use CODE PATH 1 of 2: MmapParser with mio for maximum performance.
165+
* Emscripten builds fall back to the stream-based implementation because mmap is unavailable.
166166
*
167-
* @note Bugs can exist in this path independently of the stream path (and vice versa)
168-
* @note When writing tests that validate I/O behavior, BOTH paths must be tested
169-
* @see StreamParser for the alternative implementation
167+
* @note On native builds, bugs can exist in this path independently of the stream path
168+
* @note When writing tests that validate I/O behavior, test both filename and stream constructors
169+
* @see StreamParser for the stream-based alternative
170170
*/
171171
CSVReader(csv::string_view filename, CSVFormat format = CSVFormat::guess_csv());
172172

173173
/** @brief Construct CSVReader from std::istream
174174
*
175-
* CODE PATH 2 of 2: Uses StreamParser with different internal implementation than
176-
* the memory-mapped constructor above. Issue #281 was specific to THIS path only.
175+
* Uses StreamParser. On native builds this is CODE PATH 2 of 2 and remains independent
176+
* from the filename-based mmap path. On Emscripten, the filename constructor also funnels
177+
* through this implementation.
177178
*
178179
* @tparam TStream An input stream deriving from `std::istream`
179180
* @note CSV format guessing works differently here - must manually specify dialect
180-
* @note When writing tests that validate I/O behavior, BOTH paths must be tested
181+
* @note On native builds, tests that validate I/O behavior should cover both constructors
181182
* @see MmapParser for the memory-mapped alternative
182183
*/
183184
template<typename TStream,

include/internal/data_type.hpp

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,10 +34,17 @@ namespace csv {
3434
static_assert(DataType::CSV_INT64 < DataType::CSV_DOUBLE, "Integer types should come before floating point value types.");
3535

3636
namespace internals {
37-
/** Compute 10 to the power of n */
37+
/** Compute 10 to the power of n.
38+
* Only integral exponents are supported; fractional exponents
39+
* are never needed since CSV scientific notation exponents are
40+
* always integers (enforced by the CSV_INT8..CSV_INT64 guard
41+
* in _process_potential_exponential before calling this).
42+
*/
3843
template<typename T>
3944
CSV_CONST CONSTEXPR_14
4045
long double pow10(const T& n) noexcept {
46+
static_assert(std::is_integral<T>::value, "pow10 only supports integral exponents");
47+
4148
long double multiplicand = n > 0 ? 10 : 0.1,
4249
ret = 1;
4350

@@ -186,7 +193,7 @@ namespace csv {
186193

187194
// Exponents in scientific notation should not be decimal numbers
188195
if (result >= DataType::CSV_INT8 && result < DataType::CSV_DOUBLE) {
189-
if (out) *out = coeff * pow10(exponent);
196+
if (out) *out = coeff * pow10(static_cast<long long>(exponent));
190197
return DataType::CSV_DOUBLE;
191198
}
192199

0 commit comments

Comments
 (0)