On Wed, 31 Jan 2024 04:34:52 GMT, Chris Plummer <cjplum...@openjdk.org> wrote:
>> 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`.
Well, I see now.
In this case `"[, ]"` separator should work fine.
I still think that use of Matcher is much clear - regexp defines what you are
looking for and extract the value you need. Also there is no need to split
`jstackOutput` into lines - the whole jstack output can be passed to
`Pattern.matcher`.
But I don't force you if you prefer to extract the address manually.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17479#discussion_r1473643552
