Try fixing exceptions stuff accordingly to comments in #30 and #36

[menuet]

Try a minimal fix:

• In struct InvalidPartitionName, replace member std::string_view name by char[NAME_BUFFER_SIZE] name and use a class-static make function to initialize the buffer
• In Reader::Reader, replace if (not ifc.header()) throw "file not found"; by assert(ifc.hader());
• In translate_exception(), catch the exceptions of type InvalidPartitionName and std::exception

[friendlyanon]

This could be a std::array<char, 64>.

[menuet]

ok

[friendlyanon]

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 constant 1 before 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.

[menuet]

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 :-))

[friendlyanon]

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.

[menuet]

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)

[friendlyanon]

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.

[friendlyanon]

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.

[friendlyanon]

Personal nit, but I think converting ctors should be explicit almost always.

[menuet]

Hi
I think I did all the suggested corrections (or gave justification for not doing them).
Is there a chance that this PR is accepted?
It seems not very risky and it fixes a real bug, so even if my proposed PR is not perfect, I think that it would be beneficial for anyone willing to try the project.

[cdacamar]

Hi I think I did all the suggested corrections (or gave justification for not doing them). Is there a chance that this PR is accepted? It seems not very risky and it fixes a real bug, so even if my proposed PR is not perfect, I think that it would be beneficial for anyone willing to try the project.

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 std::string is also a no-go. The original design had it be a std::string_view so that the string lifetime was governed by the system attempting to process the bad partition so having the exception object effectively duplicate the string is where we're left scratching our heads. We should have a more decided answer for you soon.