Re: RFR: 8314592: Add shortcut to SymbolLookup::find [v6]
On Thu, 18 Apr 2024 11:32:13 GMT, Per Minborg wrote: >> While `SymbolLookup` correctly uses an `Optional` return to denote whether a >> symbol has been found by the lookup or not (which enables composition of >> symbol lookups), many clients end up just calling `Optional::get`, or >> `Optional::orElseThrow()` on the result. >> >> This PR proposes to add a convenience method `SymbolLookup::findOrThrow` >> that will do a lookup and, if no symbol can be found, throws an >> `IllegalArgumentException` with a relevant exception message. > > Per Minborg has updated the pull request incrementally with one additional > commit since the last revision: > > Change exception type I'm OK with the minimal changes in the desktop code. - Marked as reviewed by prr (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18474#pullrequestreview-2009552015
Re: RFR: 8314592: Add shortcut to SymbolLookup::find [v6]
On Thu, 18 Apr 2024 11:32:13 GMT, Per Minborg wrote: >> While `SymbolLookup` correctly uses an `Optional` return to denote whether a >> symbol has been found by the lookup or not (which enables composition of >> symbol lookups), many clients end up just calling `Optional::get`, or >> `Optional::orElseThrow()` on the result. >> >> This PR proposes to add a convenience method `SymbolLookup::findOrThrow` >> that will do a lookup and, if no symbol can be found, throws an >> `IllegalArgumentException` with a relevant exception message. > > Per Minborg has updated the pull request incrementally with one additional > commit since the last revision: > > Change exception type We need a test for the new method, e.g. to check that the right exception is thrown, and the message is right. The fact that no test needed to be updated when you changed the exception type is a smell. - Changes requested by mcimadamore (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18474#pullrequestreview-2009500390
Re: RFR: 8314592: Add shortcut to SymbolLookup::find [v6]
On Thu, 18 Apr 2024 11:32:13 GMT, Per Minborg wrote: >> While `SymbolLookup` correctly uses an `Optional` return to denote whether a >> symbol has been found by the lookup or not (which enables composition of >> symbol lookups), many clients end up just calling `Optional::get`, or >> `Optional::orElseThrow()` on the result. >> >> This PR proposes to add a convenience method `SymbolLookup::findOrThrow` >> that will do a lookup and, if no symbol can be found, throws an >> `IllegalArgumentException` with a relevant exception message. > > Per Minborg has updated the pull request incrementally with one additional > commit since the last revision: > > Change exception type Marked as reviewed by jvernee (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18474#pullrequestreview-2008729407
Re: RFR: 8314592: Add shortcut to SymbolLookup::find [v6]
> While `SymbolLookup` correctly uses an `Optional` return to denote whether a > symbol has been found by the lookup or not (which enables composition of > symbol lookups), many clients end up just calling `Optional::get`, or > `Optional::orElseThrow()` on the result. > > This PR proposes to add a convenience method `SymbolLookup::findOrThrow` that > will do a lookup and, if no symbol can be found, throws an > `IllegalArgumentException` with a relevant exception message. Per Minborg has updated the pull request incrementally with one additional commit since the last revision: Change exception type - Changes: - all: https://git.openjdk.org/jdk/pull/18474/files - new: https://git.openjdk.org/jdk/pull/18474/files/2ebac9fc..e2f6c4c9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18474=05 - incr: https://webrevs.openjdk.org/?repo=jdk=18474=04-05 Stats: 4 lines in 1 file changed: 1 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/18474.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18474/head:pull/18474 PR: https://git.openjdk.org/jdk/pull/18474
