Reduce verbosity of quest's file readers - #1967
Conversation
kennyweiss
added
Quest
| quest::STLReader reader; | ||
| reader.setFileName(inputFile); | ||
| reader.read(); | ||
| SLIC_ERROR_IF(reader.read() != 0, "Failed to load STL file '" << inputFile << "'."); |
There was a problem hiding this comment.
We now require users to check the return values of several quest IO functions.
Seems like a worthwhile thing to do, but will require callers to slightly modify their code.
I can roll the [[nodiscard]] back if it's controversial.
| const c2c::LengthUnit c2cLengthUnit = toC2CLengthUnit(m_lengthUnit); | ||
|
|
||
| SLIC_INFO(fmt::format("Loading contour with {} pieces", contour.getPieces().size())); | ||
| SLIC_INFO_ROOT(fmt::format("Loading contour with {} pieces", contour.getPieces().size())); |
There was a problem hiding this comment.
@dylan-copeland -- this is the line that was getting in your way
Uses nested namespaces, [[nodiscard]], empty() instead of size() == 0, and constexpr. Updates callsites w/ [[nodiscard]] to check the return values.
f522d15 to
90872e5
Compare
| * \return A view that contains the curves. | ||
| */ | ||
| CurveArrayView getCurvesView() { return m_nurbsData.view(); } | ||
| [[nodiscard]] CurveArrayView getCurvesView() { return m_nurbsData.view(); } |
There was a problem hiding this comment.
I can understand adding [[nodiscard]] to read/write methods where there is a chance at failing due to the file system. [[nodiscard]] here seems unnecessary since you would not call these methods if you were not using the curve views for something.
There was a problem hiding this comment.
Agreed -- I'll remove [[nodiscard]] from these.
The idea was that there is no reason to call these functions if you're not using the values. But I agree they can be a bit heavy handed.
| * \return numNodes the number of nodes. | ||
| */ | ||
| int getNumNodes() const { return static_cast<int>(m_num_nodes); }; | ||
| [[nodiscard]] int getNumNodes() const { return static_cast<int>(m_num_nodes); } |
There was a problem hiding this comment.
Same [[nodiscard]] complaint here.
| [[nodiscard]] virtual int read(bool validate); | ||
|
|
||
| std::string getFileUnits() const; | ||
| [[nodiscard]] std::string getFileUnits() const; |
There was a problem hiding this comment.
Same [[nodiscard]] complaint and for the next several methods.
| const std::size_t BINARY_HEADER_SIZE = 80; // bytes | ||
| const std::size_t BINARY_TRI_SIZE = 50; // bytes | ||
| constexpr std::size_t binary_header_size = 80; // bytes | ||
| constexpr std::size_t binary_tri_size = 50; // bytes |
There was a problem hiding this comment.
When I saw the capitalized variable name it was reminiscent of a macro and made me think "this is a constant". As a lower-case variable name it made me think the variable was computed dynamically somewhere close to its use. While this may conform to some coding guideline, I find it less intuitive. My 2 cents.
This comment probably makes more sense further down where the values are used.
| union BinarySTLTri | ||
| { | ||
| std::int8_t raw[BINARY_TRI_SIZE]; | ||
| std::int8_t raw[binary_tri_size]; |
There was a problem hiding this comment.
This is another place where if I saw capital letters, I'd be more likely to think constant whereas I associate lower-case with runtime computed values.
There was a problem hiding this comment.
I agree -- I'll revert the capital -> lowecase constants
…aders/writers Keeps them for functions that return a status.
90872e5 to
388ea5b
Compare
rhornung67
left a comment
rhornung67
left a comment
There was a problem hiding this comment.
Nice changeset. Thanks @BradWhitlock
3 participants
Summary
constexpr,[[nodiscard]][[nodiscard]]changes the contract and requires callers to check the return values.