On Wed, 31 Jan 2024 03:50:29 GMT, Alex Menkov <amen...@openjdk.org> wrote:
>> Chris Plummer has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> fix spacing
>
> test/hotspot/jtreg/serviceability/sa/ClhsdbJstackWithConcurrentLock.java line
> 65:
>
>> 63: for (String line : lines) {
>> 64: if (line.contains(key)) {
>> 65: String[] words = line.split(key + "|[, ]");
>
> String.split() expect regexp as an argument
> Not sure how this code works (without escaping '$' and parentheses).
> I think it would be clearer to use standard regexp pattern/matcher, something
> like
>
>
> Pattern pattern = Pattern.compile("\\s+- <0x([0-9a-fA-F]+)>, \(a
> java/util/concurrent/locks/ReentrantLock\$NonfairSync\)");
> for (String line: lines) {
> Matcher matcher = pattern.matcher(line);
> if (matcher.find()) {
> addressString = matcher.group(1);
> break;
> }
> }
I think `key` doesn't really matter when doing the split. `key` is mainly for
finding the right line. The split is for tokenizing that line into words, and
the only parts that matters for that are ' ' and the ',' being used as word
delimiters. I think `key` should just be removed from the regex. I think the
source of the bug is code copied from ClhsdbInspect.java:
// Escape the token "Method*=" because the split method
uses
// a regex, not just a straight String.
String escapedKey = key.replace("*","\*");
String[] words = line.split(escapedKey+"|[ ]");
In this case key is `Method*=`. The code works and the escaping and searching
for `key` is necessary because we are looking for something like
`Method*=<0x00000000ffc2ed70>`, but I think this could have been simplified by
including `=` in the split pattern rather than all of `escapedKey`.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17479#discussion_r1472293801
