bneradt commented on issue #9992: URL: https://github.com/apache/trafficserver/issues/9992#issuecomment-1670377875
Reading the code, the design is for polymorphism in which various functions return a `Http3Error` base class (actually its `Http3ErrorUPtr` unique_ptr type). When there is nothing wrong, they return the `Http3NoError` child class, as observed with this ticket. I don't recommend we mix dynamic dispatch via the pointer and std::optional. It's potentially confusing: are we communicating that the std::optional underlying pointer type could be nullptr? That is, someone might reasonably ask whether there is a difference between an empty std::optional error and a std::optional containing a value which is itself a nullptr. I therefore suggest one of the following: 1. Stick with the setup as we have it, but do away with `Http3NoError` and instead return nullptr for those non-error situations. Or, 2. Convert our Http3Error setup to a single type with an enum type describing the error, or lack of error, rather than using dynamic dispatch. The class is small enough, and errors presumably rare enough, that simply creating them as values and passing them by copy should be cheap. And likely even elided in many of the returns. 3. Same as 2, but do away with the `Http3NoError` concept and have things return a `std::optional<Http3Error>`. Any of these options seem fine to me. The simplest to implement at the moment is option 1, but I'm willing to do any. @maskit : you have preferences? (Or maybe feedback that I'm missing something?) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
