<html>
  <head>
    <meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    <div class="moz-cite-prefix">Mikael,<br>
      <br>
      Sorry for the confusion. I see what you mean.<br>
      I missed that you made it consistent with the current style in
      these files.<br>
      Agree, that is the best you can do.<br>
      <br>
      Thanks,<br>
      Serguei<br>
      <br>
      On 11/26/12 2:37 PM, Mikael Vidstedt wrote:<br>
    </div>
    <blockquote cite="mid:50B3EF15.1080500@oracle.com" type="cite">
      <meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
      <br>
      Serguei,<br>
      <br>
      I'm confused. When I look at the latest webrev (<a
        moz-do-not-send="true" class="moz-txt-link-freetext"
        href="http://cr.openjdk.java.net/%7Emikael/8003879/webrev.02/">http://cr.openjdk.java.net/~mikael/8003879/webrev.02/</a>)
      I believe the indentation is correct - correct being defined as
      aligned on the first letter of the type name. Please clarify what
      you expect if you do not agree.<br>
      <br>
      Thanks,<br>
      Mikael<br>
      <br>
      <div class="moz-cite-prefix">On 11/26/2012 12:21 PM, <a
          moz-do-not-send="true" class="moz-txt-link-abbreviated"
          href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a>
        wrote:<br>
      </div>
      <blockquote cite="mid:50B3CF61.6010501@oracle.com" type="cite">
        <meta content="text/html; charset=UTF-8"
          http-equiv="Content-Type">
        <div class="moz-cite-prefix"><br>
          Looks good.<br>
          However, it'd be nice to fix the indentation commented by
          David (use frames to notice it):<br>
          <br>
          Â 
          <meta http-equiv="content-type" content="text/html;
            charset=UTF-8">
          <b>vmStructs_cms.hpp</b>:<br>
          <meta http-equiv="content-type" content="text/html;
            charset=UTF-8">
          <pre><span class="changed">68            declare_type(AFLBinaryTreeDictionary, FreeBlockDictionary&lt;FreeChunk&gt;)</span></pre>
          <br>
          Â 
          <meta http-equiv="content-type" content="text/html;
            charset=UTF-8">
          <b>jni.cpp:</b>
          <meta http-equiv="content-type" content="text/html;
            charset=UTF-8">
          <pre>2110            declare_type(MetablockTreeDictionary, FreeBlockDictionary&lt;Metablock&gt;)</pre>
          <br>
          Thanks,<br>
          Serguei<br>
          <br>
          <br>
          On 11/22/12 5:04 PM, Mikael Vidstedt wrote:<br>
        </div>
        <blockquote cite="mid:50AECB9F.8070207@oracle.com" type="cite">
          <br>
          On 2012-11-22 16:35, David Holmes wrote: <br>
          <blockquote type="cite">Hi Mikael, <br>
            <br>
            I prefer this as an internal test too. Though I don't
            understand why execute_internal_vm_tests is in jni.cpp
            rather than jvm.cpp - this is an enhancement to the VM
            interface not the JNI interface isn't it? (Separate issue of
            course). <br>
          </blockquote>
          <br>
          I think we'll find that when we get a few more tests in there
          it's time to look at breaking it out into a separate file
          completely or do it in a completely different way, but that's
          for another day. I do hope it's soon though. <br>
          <br>
          <blockquote type="cite">BTW the weird indentation makes more
            sense viewing frames rather than diffs :) <br>
          </blockquote>
          <br>
          Right :) <br>
          <br>
          <blockquote type="cite"> <br>
            Looks good to go. <br>
          </blockquote>
          <br>
          Thanks! Eagerly awaiting a second review! <br>
          <br>
          /Mikael <br>
          <br>
          <blockquote type="cite"> <br>
            David <br>
            <br>
            On 23/11/2012 10:16 AM, Mikael Vidstedt wrote: <br>
            <blockquote type="cite"> <br>
              I gave it some more thought and decided to try what it
              would look like <br>
              having the test be part of the ExecuteInternalVMTests
              family instead. I <br>
              like this one better personally, but comments are
              appreciated! <br>
              <br>
              <a moz-do-not-send="true" class="moz-txt-link-freetext"
                href="http://cr.openjdk.java.net/%7Emikael/8003879/webrev.02">http://cr.openjdk.java.net/~mikael/8003879/webrev.02</a>
              <br>
              <br>
              Cheers, <br>
              Mikael <br>
              <br>
              On 2012-11-22 14:25, Mikael Vidstedt wrote: <br>
              <blockquote type="cite"> <br>
                New webrev available here: <br>
                <br>
                <a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Emikael/8003879/webrev.01">http://cr.openjdk.java.net/~mikael/8003879/webrev.01</a>
                <br>
                <br>
                Cheers, <br>
                Mikael <br>
                <br>
                On 2012-11-22 14:19, Mikael Vidstedt wrote: <br>
                <blockquote type="cite">On 2012-11-21 20:17, David
                  Holmes wrote: <br>
                  <blockquote type="cite">Hi Mikael, <br>
                    <br>
                    On 22/11/2012 11:42 AM, Mikael Vidstedt wrote: <br>
                    <blockquote type="cite"> <br>
                      Please review the below change. <br>
                      <br>
                      The change for 7045397 introduced a couple of
                      duplicate entries in the <br>
                      vmStructs::localHotSpotVMTypes array. This shows
                      up when using the <br>
                      jmap <br>
                      tool in a rather ugly way: <br>
                    </blockquote>
                    &lt;snip&gt; <br>
                    <br>
                    Other than an indentation problem (which I don't
                    think you <br>
                    introduced) this fixup seems fine. <br>
                  </blockquote>
                  <br>
                  For some reason that's the standard for declaring
                  types in that long <br>
                  list - aligning on the first letter of the type name.
                  I followed suit. <br>
                  <br>
                  <blockquote type="cite"> <br>
                    <blockquote type="cite">In addition to removing the
                      two duplicated entries I also added a <br>
                      simple, naive runtime test to walk through and
                      make sure no type is <br>
                      repeated. The VMStructs::init only called in
                      debug_only so there's no <br>
                      startup overhead in product, but it may be better
                      to turn the test <br>
                      into <br>
                      a unit test and only running it as part of
                      ExecuteInternalVMTests. <br>
                      Feedback appreciated! <br>
                    </blockquote>
                    <br>
                    I have a few comments on this part: <br>
                    <br>
                    - Array indices should be int's not size_t (ie
                    signed not unsigned) <br>
                  </blockquote>
                  <br>
                  Will fix. <br>
                  <br>
                  <blockquote type="cite">- I can't clearly see how the
                    localHotSpotVMTypes array is declared <br>
                    or filled but I assume there is guaranteed to be a
                    sentinel entry at <br>
                    the end so that we don't index past the end? <br>
                  </blockquote>
                  <br>
                  Yeah. I'm not sure you want to know. It took me a few
                  minutes to <br>
                  figure it out. It turns out that the
                  GENERATE_VM_TYPE_LAST_ENTRY <br>
                  macro which generates the end marker is passed to
                  VM_TYPES, <br>
                  VM_TYPES_CPU and VM_TYPES_OS_CPU. But only the last
                  one is allowed to <br>
                  actually allowed to call the macro and actually
                  generate the marker, <br>
                  meaning only the "implementation" of VM_TYPES_OS_CPU
                  actually has the <br>
                  call to last_entry(). Not sure why the end marker
                  isn't just <br>
                  explicitly added at the end instead, but I think I'll
                  queue that up <br>
                  for later. <br>
                  <br>
                  All in all, as far as I can tell there is one and only
                  one end marker <br>
                  and it is generated as part of the expansion of
                  VM_TYPES_OS_CPU. <br>
                  <br>
                  <blockquote type="cite">- assert(0, ...) should be
                    assert(false, ...) (as per style guide <br>
                    [1] ;-) ) <br>
                  </blockquote>
                  <br>
                  Will fix. <br>
                  <br>
                  <blockquote type="cite">That all said I'm not sure
                    this test belongs there, but I don't feel <br>
                    strongly either way. <br>
                  </blockquote>
                  <br>
                  Me neither :) <br>
                  <br>
                  Cheers, <br>
                  Mikael <br>
                  <br>
                  <blockquote type="cite"> <br>
                    Cheers, <br>
                    David <br>
                    <br>
                    [1] <a moz-do-not-send="true"
                      class="moz-txt-link-freetext"
                      href="https://wikis.oracle.com/display/HotSpotInternals/StyleGuide">https://wikis.oracle.com/display/HotSpotInternals/StyleGuide</a>
                    <br>
                    <br>
                    <blockquote type="cite"> <br>
                      <a moz-do-not-send="true"
                        class="moz-txt-link-freetext"
                        href="http://cr.openjdk.java.net/%7Emikael/8003879/webrev.00/">http://cr.openjdk.java.net/~mikael/8003879/webrev.00/</a>
                      <br>
                      <br>
                      Cheers, <br>
                      Mikael <br>
                      <br>
                    </blockquote>
                  </blockquote>
                  <br>
                </blockquote>
                <br>
              </blockquote>
              <br>
            </blockquote>
          </blockquote>
          <br>
        </blockquote>
        <br>
      </blockquote>
      <br>
    </blockquote>
    <br>
  </body>
</html>