Fix MANIFOLD_DEBUG guards for Read/Write OBJ api #1226
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1226 +/- ##
=======================================
Coverage 92.19% 92.19%
=======================================
Files 31 31
Lines 5985 5985
=======================================
Hits 5518 5518
Misses 467 467 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| static Manifold ReadOBJ(std::istream& stream); | ||
| bool WriteOBJ(std::ostream& stream) const; | ||
| #endif |
There was a problem hiding this comment.
I'm honestly curious, from a consumer's perspective - is this more convenient? This way if you write code that uses these and then change your CMake flag, it'll suddenly stop compiling. The old way it would just fail to run. Which do we think is better behavior?
There was a problem hiding this comment.
Good question. I am a PL guy, so I want more static guarantees. But from a user perspective, failing the compilation may not be the best way...
There was a problem hiding this comment.
If the headers are going to be behind the MANIFOLD_DEBUG guard, don't these methods need to be too? Otherwise user code might not compile anyway due to not having the necessary includes?
There was a problem hiding this comment.
Oh right, the point is to not include the stream definitions, so we do have to guard them this way. I suppose finding out about problems earlier (compilation) is better than later (runtime).
| static Manifold ReadOBJ(std::istream& stream); | ||
| bool WriteOBJ(std::ostream& stream) const; | ||
| #endif |
There was a problem hiding this comment.
Oh right, the point is to not include the stream definitions, so we do have to guard them this way. I suppose finding out about problems earlier (compilation) is better than later (runtime).
| @@ -720,7 +720,6 @@ std::ostream& operator<<(std::ostream& stream, const Manifold::Impl& impl) { | |||
| * Write function. | |||
| */ | |||
| Manifold Manifold::ReadOBJ(std::istream& stream) { | |||
There was a problem hiding this comment.
Don't we have to guard the implementations the same way as the headers?
There was a problem hiding this comment.
Maybe pull master and check - I feel like this won't compile as is with MANIFOLD_DEBUG=OFF, or else I'm missing something about how these compilers work.
There was a problem hiding this comment.
Yes we do - there is an ifdef MANIFOLD_DEBUG guarding the whole block on line 685. I forgot to remove it when I added the method internal versions in the previous commit, so I didn't need to re-add it here.
There was a problem hiding this comment.
Come to think of it - do all the CI tests have MANIFOLD_DEBUG=ON?
No description provided.