Try fixing exceptions stuff accordingly to comments in #30 and #36 #52
Conversation
include/ifc/abstract-sgraph.hxx
Outdated
| static constexpr unsigned NAME_BUFFER_SIZE = 64; | ||
|
|
||
| char name[NAME_BUFFER_SIZE] = ""; |
There was a problem hiding this comment.
This could be a std::array<char, 64>.
include/ifc/abstract-sgraph.hxx
Outdated
| { | ||
| InvalidPartitionName e{}; | ||
| name = name.substr(0, NAME_BUFFER_SIZE - 1); | ||
| strncpy_s(e.name, name.data(), name.length()); |
There was a problem hiding this comment.
strncpy_s is a C11 function, takes 4 arguments and it's not guaranteed to be available: https://en.cppreference.com/w/c/string/byte/strncpy
As with all bounds-checked functions, strncpy_s only guaranteed to be available if
__STDC_LIB_EXT1__is defined by the implementation and if the user defines__STDC_WANT_LIB_EXT1__to the integer constant1before including <string.h>.
The return value is also not checked.
However, it's very silly to use this function here. This should just be a std::copy or std::memcpy.
There was a problem hiding this comment.
ok (by the way, in msvc-provided strncpy_s, the function is also a template, in order to avoid passing the size of the destination array manually, but I guess, this is an non-standardized extension on top of an already-optional extension :-))
include/ifc/abstract-sgraph.hxx
Outdated
|
|
||
| char name[NAME_BUFFER_SIZE] = ""; | ||
|
|
||
| static InvalidPartitionName make(std::string_view name) noexcept |
There was a problem hiding this comment.
Why a factory function instead of an explicit ctor? Copying from one buffer into another can't fail, so there is no reason to make this a factory function.
There was a problem hiding this comment.
In one of my previous attempts (#30 (comment)), @cdacamar told me not to introduce a ctor in this class (but maybe his the rationale does not apply if we have some real work to do anyway in the ctor)
There was a problem hiding this comment.
The comment was on an implicit converting ctor that did the same thing the compiler generated one would have, so that comment was valid. However, here the work is non-trivial, so this can be a ctor.
…buffer private to enforce the invariant of always ending with 0)
| { | ||
| partition_name = partition_name.substr(0, partition_name_buffer.size() - 1); | ||
| std::copy(partition_name.begin(), partition_name.end(), partition_name_buffer.begin()); | ||
| partition_name_buffer[partition_name.length()] = 0; |
There was a problem hiding this comment.
partition_name is already sized in such a way that the last byte in the buffer stays the way it was initiliazed, which is 0. This line is unnecessary.
include/ifc/abstract-sgraph.hxx
Outdated
| @@ -3544,7 +3546,20 @@ namespace ifc { | |||
|
|
|||
| // -- exception type in case of an invalid partition name | |||
| struct InvalidPartitionName { | |||
| std::string_view name; | |||
| InvalidPartitionName(std::string_view partition_name) | |||
There was a problem hiding this comment.
Personal nit, but I think converting ctors should be explicit almost always.
|
Hi |
Indeed, I think the fixes here are worth evaluating. I have been talking with @GabrielDosReis and we plan on reviewing the changes here sometime later this week or early next week--apologies for the delays. Gaby is unsure about the static buffer change in the exception object, but dynamic memory via something like |
Try a minimal fix:
struct InvalidPartitionName, replace memberstd::string_view namebychar[NAME_BUFFER_SIZE] nameand use a class-staticmakefunction to initialize the bufferReader::Reader, replaceif (not ifc.header()) throw "file not found";byassert(ifc.hader());translate_exception(), catch the exceptions of typeInvalidPartitionNameandstd::exception