src: fix useless call in permission.cc - #46833
Conversation
Sorry, something went wrong.
This comment was marked as outdated.
addaleax
left a comment
There was a problem hiding this comment.
I agree that the current state here isn't ideal, but I'd rather fix it by doing proper error handling (i.e. return; on empty Maybe) than using .Check() and adding potential crashes here
Sorry, something went wrong.
|
@addaleax I went with Check() because the same statements already use ToLocalChecked(). Would you prefer to replace only Check() or both functions with proper error handling? |
Sorry, something went wrong.
|
@tniessen I think in ideal world we'd also handle errors from the string creation functions, but since they can only fail (afaik) if the strings exceed the maximum length of strings allowed by V8, and they (currently) only accept input that is already defined at compile time, so I don't have strong feelings about those (The extra std::string() call here is another story, but... 🤷♀️) |
Sorry, something went wrong.
|
@addaleax Thank you, that makes sense. I'll update it soon. |
Sorry, something went wrong.
FromMaybe() has no side effects and the return value is ignored. Instead, if Set() fails, then another exception is pending, so return early.
Sorry, something went wrong.
Sorry, something went wrong.
FromMaybe() has no side effects and the return value is ignored. Instead, if Set() fails, then another exception is pending, so return early. PR-URL: #46833 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
FromMaybe() has no side effects and the return value is ignored. Use Check() instead to ensure that the operation succeeded.