header: getting rid of exception-throwing behaviors in header files [include/ and source/common/]#12502
Conversation
Signed-off-by: Yifan Yang <needyyang@google.com>
include/envoy/buffer/buffer.h
Outdated
| * more than a single PR. | ||
| */ | ||
| template <typename T, ByteOrder Endianness = ByteOrder::Host, size_t Size = sizeof(T)> | ||
| StatusOr<T> peekIntNoExcept(uint64_t start = 0) { |
There was a problem hiding this comment.
drive by: instead of large changes like this can't you just throw the exception from the cc file? I don't see why these changes are necessary. Actually removing exceptions from control flow is orthogonal. Let's please keep the changes focused on just removing exceptions from header files which generally should be trivial.
There was a problem hiding this comment.
Working with template functions is a little different as we will need to do explicit template instantiations to move the template function implementations into .cc files. That also means that we need to instantiate all the currently used type in .cc files as such:
template a_lot_of_types peekInt(uint64_t start);
But yeah, if you believe this way is better than what my current method is, I am certainly on board with it as it makes the change much narrower and cleaner (instead of going around changing all call sites).
There was a problem hiding this comment.
I don't even think that is necessary. Just define a non-templated helper which throws the exception and implement it in a cc file. This will fix the immediate problem if no throwing from header files very quickly and get the linter up. Now, whether this fixes the real problem in the sense that you can't through through header files is a different issue, but I would cross that bridge when you come to it.
There was a problem hiding this comment.
I think that is a great way to do this! Wonder why that didn't become obvious to me. But I have just heard that @asraa is trying to get rid of the exception throwing behavior altogether in the dataplane pipeline. So maybe she has some input on this topic.
But this is definitely a much better way of doing this.
There was a problem hiding this comment.
But I have just heard that @asraa is trying to get rid of the exception throwing behavior altogether in the dataplane pipeline. So maybe she has some input on this topic.
As I said above, I think removing exceptions from certain code paths is orthogonal to this effort. My advice is to start by meeting your immediate goal in as simple a way as possible and then we can go from there?
Signed-off-by: Yifan Yang <needyyang@google.com>
|
@yanavlasov could you take a look? |
|
LGTM, I'll re-approve once format is fixed. |
Signed-off-by: Yifan Yang <needyyang@google.com>
Signed-off-by: Yifan Yang <needyyang@google.com>
Signed-off-by: Yifan Yang <needyyang@google.com>
…_format script Signed-off-by: Yifan Yang <needyyang@google.com>
|
@yanavlasov do you mind taking a look? I believe the only test that fails here is a flaky one. |
mattklein123
left a comment
There was a problem hiding this comment.
LGTM with one small comment, thanks!
/wait
source/common/config/grpc_mux_impl.h
Outdated
| ExceptionUtil::throwEnvoyException("ADS must be configured to support an ADS config source"); | ||
| // it should never reach here but the compiler complains and the alternative will be to change | ||
| // the return type of the parent virtual function to absl::<optional<current_return_type>> | ||
| return nullptr; |
There was a problem hiding this comment.
Use NOT_REACHED here. I think you may also be able to use some type of "never returns" annotation on throwEnvoyException
There was a problem hiding this comment.
used noreturn keyword on function signature
Signed-off-by: Yifan Yang <needyyang@google.com>
Signed-off-by: Yifan Yang <needyyang@google.com>
Signed-off-by: Yifan Yang needyyang@google.com
This is a part of efforts to get rid of all the exception throwing semantics in envoy header files, as discussed here #12469. This PR deals with all instances that throw plain envoy exceptions in the described sub modules.
Commit Message:
Additional Description:
Risk Level: Low. No new mechanics except the additional status check is introduced.
Testing:
Docs Changes:
Release Notes:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]