RFR: 8260198: TypeInstPtr::dump2() emits multiple lines if Verbose is set [v8]

[Evgeny Astigeevich]

On Wed, 24 Feb 2021 09:50:04 GMT, Xin Liu <xliu at openjdk.org> wrote:

>> Add a flag _suppress_cr to outputStream. outstream objects won't emit any CR if it's set.
>> Correct TypeInstPtr::dump2 to make sure it only emits klass name once.
>> Remove the comment because Klass::oop_print_on() has emitted the address of oop.
>> 
>> Before:
>> 689  ConP  ===  0  [[ 821 ]]   Oop:java/lang/Stringjava.lang.String
>> {0x000000010159d7c8} - klass: public final synchronized 'java/lang/String'
>> - string: "a"
>> :Constant:exact *
>> 
>> After:
>> 689  ConP  ===  0  [[ 821 ]]   Oop:java.lang.String {0x000000010159d7c8} - klass: public final synchronized 'java/lang/String' - string: "a":Constant:exact *
>
> Xin Liu has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8260198: TypeInstPtr::dump2() emits multiple lines if Verbose is set
>   
>   add comments and hoist ResourceMark

Changes requested by eastig at github.com (no known OpenJDK username).

src/hotspot/share/opto/type.cpp line 4053:

> 4051:       const_oop()->print_oop(&ss);
> 4052:       // suppress new-lines('\n') in ss emitted by const_oop->print_oop()
> 4053:       // so each node is one-liner for -XX:+Verbose && -XX:+PrintIdeal

What about rewriting the comment in clearer way:
// 'const_oop->print_oop()' emits new-lines('\n') into ss.
// For -XX:+Verbose && -XX:+PrintIdeal, new-lines('\n') must be removed from
// the ss created string to have a node per line.

test/hotspot/gtest/utilities/test_ostream.cpp line 66:

> 64: 
> 65: static size_t count_char(const stringStream* ss, char ch) {
> 66:   return count_char(ss->as_string(), ss->size(), ch);

Am I correct `std:count` is not allowed?
No need to use `as_string`: `return count_char(ss->base(), ss->size(), ch);`
Or as `stringStream` is always zero-terminated: `return count_char(ss->base(), ch);`

test/hotspot/gtest/utilities/test_ostream.cpp line 72:

> 70:   ResourceMark rm;
> 71:   size_t whitespaces = count_char(ss, ' ');
> 72:   char* s2 = ss->as_string(false);

No need of `false` because `false` is the default value of `as_string`. If you want to be explicit here, I recommend: `char* s2 = ss->as_string(/* c_heap= */ false);`

test/hotspot/gtest/utilities/test_ostream.cpp line 63:

> 61:   }
> 62:   return cnt;
> 63: }

As the function is only used for zero-terminated strings, maybe it makes sense to use this property:
static size_t count_char(const char* s, char ch) {
   size_t cnt = 0;

   while (*s != '\0') {
      if (*s++ == ch) {
        ++cnt;
      }
   }
   return cnt;
}

test/hotspot/gtest/utilities/test_ostream.cpp line 69:

> 67: }
> 68: 
> 69: static void test_stringStream_tr_delete(stringStream* ss) {

I think this is a unit test for `StringUtils::replace_no_expand`. It checks that the function can be used to remove substrings. There is no dependency on `stringStream`. Any string can be used.
Could you please move the test to `test_stringUtils.cpp`?

-------------

PR: https://git.openjdk.java.net/jdk/pull/2178