quick fix for non-handled exception InvalidPartitionName

[menuet]

When trying to run ifc-printer against a sample ifc file, I encountered this uncaught exception : InvalidPartitionName.
So, I added it inside translate_exception. (I had to change the type of name from string_view to string in order to avoid a memory issue because the string_view pointed to a no-more-allocated string at the time I am displaying it)
For the sample ifc file that provoked the error, I used the file file.cxx and generated the related ifc file with msvc 17.7.4 and this command line :
cl /std:c++20 /exportHeader file.cxx /I [myworkingdir]\ifc\include /I [mybuilddir]\x64-Debug_deps\gsl-src\include

[cdacamar]

I think we want to keep this as an aggregate type.

[menuet]

OK, thanks.
At least, we have to change the name's type to be std::string and to change the call site to be: throw InvalidPartitionName{std::string{name}}, because at the time it is catched it points to released memory.
By the way, why is it important to keep struct InvalidPartitionName as an aggregate type?
Actually, as a more general topic, I don't understand why the are several different ways of creating exception objects in the project : sometimes they are just struct, sometime they are plain string literals, sometime they are classes deriving from std::logic_error, ...
(Anyway, thanks for open-sourcing this project, this seems really interesting and promising)

[cdacamar]

At least, we have to change the name's type to be std::string and to change the call site to be: throw InvalidPartitionName{std::string{name}}, because at the time it is catched it points to released memory.

Yes, definitely agreed!

By the way, why is it important to keep struct InvalidPartitionName as an aggregate type?

In a word: simplicity. If there is no reason to make the structure a non-aggregate, then we should avoid introducing the overhead of maintaining a ctor that would have done the work the compiler could have done automatically for us, e.g. as an aggregate, we can move-from a std::string, which we would have to create a new overload for if we introduce a ctor.

Actually, as a more general topic, I don't understand why the are several different ways of creating exception objects in the project : sometimes they are just struct, sometime they are plain string literals, sometime they are classes deriving from std::logic_error, ...

This is a good observation. It would be good for us to unify the exception types around the project. @gdr-at-ms do you have thoughts here?

[menuet]

I will create a branch in my fork where I make all the exceptions objects of classes deriving from std::exception.
Then, if you tell me that this is the direction that you want, I will propose a PR.

[GabrielDosReis]

This is a good observation. It would be good for us to unify the exception types around the project. @gdr-at-ms do you have thoughts here?

Distinct types for distinct erroneous conditions. They are not to be derived from std::exception hierarchy.

[menuet]

Wouldn't it be good to have distinct types all deriving indirectly from std::exception?
What is the rationale of not letting these types derive from a common type?
This would be easier for the call site to be able to handle all the cases by catching std::ifc_exception& (and also being able to be more specific if desired)
I ask because, right now, in the function translate_exception, there is not any catch(InvalidPartitionName&) therefore, when trying the ifc-printer, I got a very vague error message "unknown exception caught" instead of having something like "caught: ifc invalid partition name: 'expr.class-subobject-value'"

[GabrielDosReis]

Wouldn't it be good to have distinct types all deriving indirectly from std::exception?

No, that is considered a mistake by modern guidelines.

I got a very vague error message "unknown exception caught" instead of having something like "caught: ifc invalid partition name: 'expr.class-subobject-value'"

I get that, but a common base class wouldn't have necessarily fix the real issue - it would just have papered over it.

[menuet]

Looking at the C++ core guidelines, it is not clear for me whether it is good practice or bad practice to derive from a common type: E.14: Use purpose-designed user-defined types as exceptions (not built-in types)
At least, they state that we should not throw built-in types like ints or char*.
But in reader.cxx, the code does: throw "file not found";
Anyway, my original PR is just about taking the exception InvalidPartitionName into account in translate_exception. If I rewrite my PR so that the type InvalidPartitionName stays as an aggregate, would it be ok for you ?

[GabrielDosReis]

But in reader.cxx, the code does: throw "file not found";

That is clearly a mistake that should have been caught before it was checked in.

If I rewrite my PR so that the type InvalidPartitionName stays as an aggregate, would it be ok for you ?

Yes!

And in separate PRs, we should clean up the rest of the issues. Thanks for working on this; very much appreciate.

[cdacamar]

I would recommend opening up a separate issue around the unification of exception types. It's not clear to me that we actually do want to inherit from std::exception in each type (it has its own caveats such as exception handling order then becomes far more important vs unique types without a common base).

I was reflecting on the original PR content, which is noble: let's make the exception object holding data have clear guarantees around the held data lifetime. std::string_view is an obvious indicator that the lifetime of the string content living elsewhere, in this case it is always going to be data coming from the file held in a separate buffer somewhere which also holds the content of the file. From this perspective, std::string actually becomes unnecessary overhead because we'd be reallocating space for a string we've already allocated space for.

That said, there's still the scenario where you want to 'probe' for partitions which may/may not exist in which case you may have something like:

try {
  std::string test_partition = "foobar";
  auto p = offset(offset_table, test_partition);
} catch (const InvalidPartitionName& e) { /* e.name is invalid here */ }

Now that said, offset inside of sgraph.cxx is a private implementation and that is the only place, currently, where InvalidPartitionName is thrown and those strings are statically allocated at program load time. Can you walk me through how you discovered the string lifetime problem on your end? That might help us better understand what the correct fix should be.