Skip to content

Conversation

@ThePseudo
Copy link
Contributor

In this PR, I add noexcept to all constructors that are noexcept

@ThePseudo ThePseudo marked this pull request as draft February 6, 2025 16:20
@ThePseudo
Copy link
Contributor Author

@axsaucedo I prepared this PR for #44 but I think there is a design problem to do this. I found this in the failed test because now Memory fails, throws an exception but it gets converted to a segfault in the Tensor class (as it should be).

However, I do not feel right to delete the test for the Tensor/Memory data to be >0 in size or delete the check inside of the tensor.
One easy solution might be not having the Tensor being noexcept, and that would be all.

However, I also do not feel this to be completely right, and as the tensor size should be constant I feel that it should be possible to check it, and (this is really a stretch) I feel that this could be possible at compile time through templates, although I am not sure.

What do you suggest?

@ThePseudo ThePseudo force-pushed the noexcept branch 2 times, most recently from 1969b2a to 2dc1f95 Compare February 10, 2025 07:52
Signed-off-by: Andrea Calabrese <[email protected]>
Signed-off-by: Andrea Calabrese <[email protected]>
Signed-off-by: Andrea Calabrese <[email protected]>
Signed-off-by: Andrea Calabrese <[email protected]>
Signed-off-by: Andrea Calabrese <[email protected]>
Signed-off-by: Andrea Calabrese <[email protected]>
@ThePseudo ThePseudo marked this pull request as ready for review February 10, 2025 07:56
@axsaucedo
Copy link
Member

Thank you for the contribution @ThePseudo !

Additionally I agree with your comments; to be honest it's been well overdue to properly think about having a better custom exception handling approach, as currently there's quite a few situations where the program exits where a domain-specific exception would be better handled.

One easy solution might be not having the Tensor being noexcept, and that would be all.
For the purpose of this PR I would say we can go forward with this, and then revisit in context of the above; likely this may end up as a bit of a exception to the rule, but let's explore if it can be done (+ wthout a more complex approach).

Based on that let me know and we can merge this one to approach separate.

@ThePseudo
Copy link
Contributor Author

Hi @axsaucedo, I think it is possible to merge this one. I was experimenting with the other solution and I don't like where it was going: basically it would become a template hell, which would even be ok by me, but everything becoming statically decided does not work for everything (memory usage would be decided at compile time, basically).
My feeling is that I would like to not have exceptions, but in the end it is the most reasonable solution C++ offers us.

@axsaucedo
Copy link
Member

My feeling is that I would like to not have exceptions, but in the end it is the most reasonable solution C++ offers us.
Agreed - but for this context specifically, I meant to clean up the areas in the code where we throw a std::runtime_error as to be honest a more specific exception would be more reasonable.

In regards to this PR looks good to go, thanks for the contribution

@axsaucedo axsaucedo merged commit 11c0b99 into KomputeProject:master Feb 11, 2025
8 checks passed
@ThePseudo ThePseudo deleted the noexcept branch February 11, 2025 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants