Skip to content

Fix open review comments on MeshIO PR#18

Merged
csparker247 merged 2 commits intofeature/mesh-io_20260323from
copilot/fix-open-comments
Apr 23, 2026
Merged

Fix open review comments on MeshIO PR#18
csparker247 merged 2 commits intofeature/mesh-io_20260323from
copilot/fix-open-comments

Conversation

Copy link
Copy Markdown

Copilot AI commented Apr 23, 2026

Addresses all unresolved review comments on the MeshIO implementation PR.

MeshIO_PLY.hpp

  • PLYType enum replaces std::string for PLYProp::type and list_count_type; parse_ply_type() converts header tokens at parse time. ply_type_bytes, read_ply_binary_prop, and read_ply_prop_from_buf now dispatch via switch instead of sequential string comparisons
  • Color-type conditions now match only PLYType::UChar / PLYType::UShort; removed signed char/short cases that could produce incorrect casts for negative values
  • trim_right (string_view) replaces trim_right_in_place in all three ASCII read loops; the trimmed string_view is passed directly to split, avoiding a redundant cast

CMake charconv probes

  • CheckCharconvFP.cmake now appends needed definitions to EDUCE_CORE_CHARCONV_DEFS instead of calling add_compile_definitions
  • CMakeLists.txt applies them via target_compile_definitions(core PUBLIC ...), so they propagate automatically to any downstream target via target_link_libraries
# No manual propagation needed anymore
target_link_libraries(foo PRIVATE educelab::core)

Miscellaneous

  • Math.hpp: cross() error message → "Inputs must be 3-dimensional"
  • String.hpp: comment at predicate-dispatch call site clarifying it routes to the is_invocable_r_v-constrained overload, not back into the variadic template
  • README.md: charconv section updated with correct per-type definition names (EDUCE_CORE_NEED_FROM/TO_CHARS_{FLOAT,DOUBLE,LONG_DOUBLE}) and revised cmake example reflecting automatic propagation
  • conductor/product.md: trimmed goal (String) Add partition and to_padded_string #4 per suggestion

Copilot AI and others added 2 commits April 23, 2026 18:52
- conductor/product.md: trim goal #4 to one line (remove vertex-trait detail per suggestion)
- Math.hpp: cross() error message → "Inputs must be 3-dimensional"
- String.hpp: add comment confirming predicate-overload dispatch at line 266
- cmake/CheckCharconvFP.cmake: collect fallback defs into EDUCE_CORE_CHARCONV_DEFS list
  instead of add_compile_definitions; caller applies them via target_compile_definitions
- CMakeLists.txt: target_compile_definitions(core PUBLIC ${EDUCE_CORE_CHARCONV_DEFS})
  so definitions propagate automatically to downstream targets
- README.md: update charconv section — correct per-type definition names, show automatic
  propagation via target_link_libraries, keep header-only manual example
- MeshIO_PLY.hpp: introduce PLYType enum + parse_ply_type(); replace std::string type /
  list_count_type fields in PLYProp; convert ply_type_bytes, read_ply_binary_prop,
  read_ply_prop_from_buf to switch-on-enum dispatch
- MeshIO_PLY.hpp: use trim_right (string_view) in skip_ascii_line, vertex ASCII loop,
  and face ASCII loop instead of trim_right_in_place
- MeshIO_PLY.hpp: color-type conditions use PLYType::UChar / PLYType::UShort only
  (remove signed char/short cases that could produce incorrect casts)

Agent-Logs-Url: https://github.com/educelab/libcore/sessions/7a14eef7-28d9-4b16-98ba-a8ef620e0c9f

Co-authored-by: csparker247 <1434526+csparker247@users.noreply.github.com>
@csparker247 csparker247 marked this pull request as ready for review April 23, 2026 19:01
@csparker247 csparker247 merged commit 7d6254f into feature/mesh-io_20260323 Apr 23, 2026
@csparker247 csparker247 deleted the copilot/fix-open-comments branch April 23, 2026 19:01
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.

2 participants