Conversation
…II-GF Mesh_2: Fix write_vtu in ASCII
| template <typename Point> | ||
| bool read_OFF(std::istream& is, | ||
| Surface_mesh<Point>& sm) | ||
| { | ||
| return read_OFF(is, sm, parameters::all_default()); | ||
| } | ||
| template <typename Point, | ||
| typename CGAL_BGL_NP_TEMPLATE_PARAMETERS> | ||
| bool read_OFF(const char* fname, | ||
| Surface_mesh<Point>& sm, | ||
| const CGAL_BGL_NP_CLASS& np) | ||
| { | ||
| std::ifstream in(fname); | ||
| return read_OFF(in, sm, np); | ||
| } | ||
|
|
||
| template <typename Point, | ||
| typename CGAL_BGL_NP_TEMPLATE_PARAMETERS> | ||
| bool read_OFF(const char* fname, | ||
| Surface_mesh<Point>& sm) | ||
| { | ||
| return read_OFF(fname, sm, parameters::all_default()); | ||
| } | ||
|
|
||
|
|
||
| template <typename Point, | ||
| typename CGAL_BGL_NP_TEMPLATE_PARAMETERS> | ||
| bool read_OFF(std::string& fname, | ||
| Surface_mesh<Point>& sm, | ||
| const CGAL_BGL_NP_CLASS& np) | ||
| { | ||
| return read_OFF(fname.c_str(), sm, np); | ||
| } | ||
|
|
||
| template <typename Point, | ||
| typename CGAL_BGL_NP_TEMPLATE_PARAMETERS> | ||
| bool read_OFF(std::string& fname, | ||
| Surface_mesh<Point>& sm) | ||
| { | ||
| return read_OFF(fname, sm, parameters::all_default()); | ||
| } | ||
|
|
There was a problem hiding this comment.
There must be something really wrong with that API, if one needs to define so many overloads!
There was a problem hiding this comment.
WELL, normally BGL would take care of all the extra and only the overload that's already here is necessary, but with the CGAL::IO:: namespace and this ADL problem we discussed, it seems that now, ALL OF THE OVERLOADS need to be defined, so yes, there is something really wrong.
There was a problem hiding this comment.
Is there a conclusion on the "really wrong" aspect? (23be657 handles it and everything is figured out?)
There was a problem hiding this comment.
There is no conclusion. I am sorry if any of you two felt attacked by my comment. It was just a statement, without any proposed solution.
The issue seems to be a problem a double-dispatch, not with dynamic polymorphism but with templates. The main key of that double-dispatch is the face-graph type. So maybe the facing function templates in CGAL::IO:: could be generic for the first parameter:
template <typename Input, typename Face_graph>
bool read_OFF(Input&& input,
Face_graph& graph);
template <typename Input, typename Point
bool read_OFF(Input&& input,
Surface_mesh<Point>& graph);
// ...and then, those function templates could dispatch again on the first parameter Input input calling functions in another namespace (CGAL::IO::internal::?)...
But I would recommend to refactor that way only for CGAL-5.4. That seems too late for CGAL-5.3. And anyway, the refactoring would be compatible with the API of CGAL-5.3.
... I am also surprised to see non-const references as parameter: std::string& fname. That seems to limit how users can call the function, and it should be fixed, in my opinion.
There was a problem hiding this comment.
No, I was just confused how moving namespaces across all functions could introduce such ADL issues that require these new overloads. But I haven't looked into it in detail (and so I was just wondering if the problem was already solved).
The string parameter should indeed be a const&.
|
|
|
Successfully tested in https://cgal.geometryfactory.com/CGAL/testsuite/results-5.3-Ic-128.shtml |
Move documented function/class in subdirectories
CGAL/IOinto theCGAL::IOnamespaceFollow up of #4255