<html>
  <head>
    <meta content="text/html; charset=gb18030" http-equiv="Content-Type">
  </head>
  <body text="#000000" bgcolor="#FFFFFF">
    Hi Sergey,<br>
      I've updated my code according to your comments. Please take a
    look again.<br>
    <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~pzhang/Charlie/TypeAnnotation1/webrev/">http://cr.openjdk.java.net/~pzhang/Charlie/TypeAnnotation1/webrev/</a><br>
    <br>
    P.S. When updating Helper.isPkgInfoPresent with: return
    files.stream().map(JavaFileObject::getName).anyMatch(name -&gt;
    name.contains("package-info") || name.contains("PkgAnno")); <br>
    There is a compile error. So the code stays the original way. Due to
    the urgency of the test, I will file a RFE bug to track it and
    update the piece of code when time allows.<br>
    <br>
    <br>
    <br>
    - Charlie<br>
    <br>
    <br>
    <div class="moz-cite-prefix">On 2013/8/27 2:27, Sergey Grinev wrote:<br>
    </div>
    <blockquote cite="mid:521B9E2B.1000205@oracle.com" type="cite">
      <meta content="text/html; charset=gb18030"
        http-equiv="Content-Type">
      Hi Charlie,<br>
      <br>
      please, take a look at few inline comments.<br>
      <br>
      -- Sergey<br>
      <br>
      <div class="moz-cite-prefix">On 26-Aug-13 11:45, Charlie Wang
        wrote:<br>
      </div>
      <blockquote cite="mid:521B07A4.8020306@oracle.com" type="cite">Hi
        Alex, <br>
          I think I will need to make update according to Sergey's
        comments before check in the code. <br>
        <br>
        Sergey, <br>
          Thanks for the comments. My reply inline. <br>
        <br>
        On 2013/8/23 18:36, Sergey Grinev wrote: <br>
        <blockquote type="cite">Hi, all. <br>
          <br>
          Sorry if this review covers same issue I reported on
          reviewboard one, but it seems few important point were not
          fixed: <br>
          <br>
          1.  public Map&lt;Helper.Declaration, Object&gt; lhm = new
          LinkedHashMap&lt;&gt;(); <br>
          and <br>
              public synchronized static String genDeclaration( <br>
                      Map&lt;Helper.Declaration, Object&gt; input,
          Declaration... srcType) <br>
          <br>
          - fields used as a method-local temporary variables are
          error-prone and confusing. Users/readers of that code are not
          aware when this field can be used or when this field can be
          cleared, also using such field is not thread-safe. Imagine
          someone added one more ENUM value to TestCase, used lhm and
          didn't call lhm.clear() -- things will break and very
          different place this person didn't even touch. <br>
          <br>
          - Map claims it stores Objects but actually they are only
          String and String[]. Thus your getDeclaration() has invalid
          signature and is error-prone. Also you calls instanceof
          several times to resolve this ambiguity which is considered to
          be an anti-pattern as well. <br>
          <br>
          You can solve these problems by introducing separate entity
          responsible for declaration generation: <br>
          <br>
          public class DeclarationGenerator { <br>
          <br>
              private final Map&lt;Helper.Declaration, String[]&gt; lhm
          = new LinkedHashMap&lt;&gt;(); <br>
                  public void add(Helper.Declaration key, String...
          value) { <br>
                  lhm.put(key, value); <br>
              } <br>
              public String genDeclaration(Declaration... srcType) { <br>
                  //... <br>
                  for (Declaration st : Declaration.values()) { <br>
                          if (input.containsKey(st)) <br>
                                 
          result.append(st.getSrc(input.get(st))); <br>
                  } <br>
                  //... <br>
              } <br>
          } <br>
          <br>
          with this helper class instead of <br>
          <br>
                          lhm.clear(); <br>
                          lhm.put(Helper.Declaration.IMPORT, null); <br>
                          lhm.put(Helper.Declaration.CLASS, str[0]); <br>
                          testCode += Helper.genDeclaration(lhm); <br>
          <br>
          you can write <br>
          <br>
                          DeclarationGenerator dg = new
          DeclarationGenerator(); // not to reuse, create new one for
          each generation <br>
                          dg.put(Helper.Declaration.IMPORT, null); <br>
                          dg.put(Helper.Declaration.CLASS, str[0]); <br>
                          testCode += dg.genDeclaration(); <br>
          <br>
          having clean design without field used as temporary variable
          and type ambiguity. <br>
          <br>
        </blockquote>
        I agree with you. I will update the code accordingly. <br>
        <br>
        <blockquote type="cite"> <br>
          2. several TestCase enum entries, especially TESTBASECLSSTART
          and TESTBASECLS are copypasted in several places. <br>
        </blockquote>
        I will remove the duplications. <br>
      </blockquote>
      <br>
      You may want to consider using separate enum for all your cases
      (or one per package).<br>
      <br>
      <blockquote cite="mid:521B07A4.8020306@oracle.com" type="cite"> <br>
        <blockquote type="cite"> <br>
          3. public static Map&lt;String, String&gt; testInput = new
          LinkedHashMap&lt;&gt;(); <br>
          <br>
          why this field in static? <br>
          <br>
        </blockquote>
        Made it static for enum methods to access directly. <br>
      </blockquote>
      <br>
      Oh I missed usages of this field, sorry. Current approach isn't
      the best from OOD point of view. You have static field cleaned in
      constructor and field being reused from the code outside of the
      AnnotationTest. This leads to uncontrollable logic flow and
      impossibility of parallel invocation.<br>
      <br>
      The convenient approach would be <br>
      1) either having this field non-static and provide instance of
      AnnotationTest or testInput as parameter to each genTestCase()
      call.<br>
      2) or using anonymous classes (or even lambdas) instead of enum,
      which allows convenient access to "this" without statics:<br>
      <br>
          private final static TestCaseGenerator TESTBASECLSSTART1 =
      (String str) -&gt; {<br>
              testInput.put(null, null); // testInput not static<br>
      <br>
              // other code<br>
      <br>
              return "";<br>
          };<br>
      <br>
      P.S.: AnnotationTest.GenTestCode() method should be <b>g</b>enTestCode()

      by common Java code guidelines.<br>
      <br>
      <blockquote cite="mid:521B07A4.8020306@oracle.com" type="cite"> <br>
        <br>
        And as to the other comments from you: <br>
        <br>
        <blockquote type="cite">1. Helper#getAnno() <br>
          <br>
            StringBuffer result = new StringBuffer(""); <br>
          <br>
          using StringBuilder is recommended for thread-safe methods <br>
        </blockquote>
        Agree. <br>
        <br>
        <blockquote type="cite"> <br>
          2. List Helper.getMultipleAT() <br>
          <br>
          this method uses List where List&lt;String&gt; can be
          specified <br>
        </blockquote>
        Good check. <br>
        <br>
        <blockquote type="cite"> <br>
          3. Helper.compareAnnoWithDiffSeq() <br>
          <br>
          this can be done much shorted and without copying array to
          list: <br>
          <br>
                  String[] split1 = anno1.trim().split("\\s+"); <br>
                  String[] split2 = anno2.trim().split("\\s+"); <br>
                  Arrays.sort(split1); <br>
                  Arrays.sort(split2); <br>
                  return Arrays.equals(split1, split2); <br>
        </blockquote>
        OK. <br>
        <br>
        <blockquote type="cite"> <br>
          4. Helper.compileCode():697-704 can be simplified using jdk8
          streams: <br>
          <br>
          ok = diagnostics.getDiagnostics().stream().anyMatch(d -&gt;
          d.getKind() == Diagnostic.Kind.ERROR); <br>
        </blockquote>
        I didn't adopt it because I wasn't sure if I supposed to use
        this new jdk8 feature in this jdk8 test. Bus since Tristan also
        suggested it is doable, I will update my code this way. <br>
        <br>
        <blockquote type="cite"> <br>
          5. Same for Helper.isPkgInfoPresent: <br>
          <br>
          return
          files.stream().map(JavaFileObject::getName).anyMatch(name
          -&gt; name.contains("package-info") ||
          name.contains("PkgAnno")); <br>
        </blockquote>
        OK. <br>
        <br>
        <blockquote type="cite"> <br>
          6. AnnotatedArrayTypeGetAnnotatedGenericComponentTypeTest:175
          <br>
          <br>
          You are calling Helper.getArrAT(at) then split it by ";" and
          work with result. <br>
          while Helper.getArrAT() takes a list and convert it into
          string: <br>
          return getAnno(annotations) + ";" + ret; <br>
          <br>
          It may be more efficient to work directly with lists. <br>
        </blockquote>
        The expected results (to be compared with test results) also
        come as strings because they are directly extracted from the
        test input, which is why I made it "getAnno(annotations) + ";" +
        ret" - so I don't have to split the result into lists. Either
        way, the conversion has to be made. The present way is more
        direct. <br>
        <br>
        <blockquote type="cite"> <br>
          7. DeclarationGenerator interface is never used as interface <br>
        </blockquote>
        I don't think it hurts to put an interface there. At least it
        will help extension. <br>
      </blockquote>
      Agree.<br>
      <blockquote cite="mid:521B07A4.8020306@oracle.com" type="cite"> <br>
        <blockquote type="cite"> <br>
          8. Helper.Declaration.replaceAnnoStr should be static <br>
        </blockquote>
        Isn't it static already. <br>
      </blockquote>
      <blockquote><a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Epzhang/Charlie/TypeAnnotation1/webrev/raw_files/new/test/type-annotations/Helper.java">http://cr.openjdk.java.net/~pzhang/Charlie/TypeAnnotation1/webrev/raw_files/new/test/type-annotations/Helper.java</a><br>
        <br>
        <pre style="color: rgb(0, 0, 0); font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; word-wrap: break-word; white-space: pre-wrap;">private String[] replaceAnnoStr(String ss, String temp,</pre>
      </blockquote>
      <br>
      Seems no.<br>
      <blockquote cite="mid:521B07A4.8020306@oracle.com" type="cite"> <br>
        <blockquote type="cite"> <br>
          9. Helper.Declaration.replaceAnnoStr/getFirstAnno return
          result as String[] array, which is considered to be a bad
          practice (you are using variable length structure to store
          exactly 2 values). It would be cleaner to introduce
          intermediate structure of 2 string to store return value. <br>
        </blockquote>
        Then I will have to declare a new class for the two strings.<br>
      </blockquote>
      <br>
      But that's the point. It would be class with fields with
      meaningful names instead of [0] and [1]. Anyone who will be
      supporting/fixing/using your code in future would be grateful for
      that. And you will avoid using array to store fixed length data
      which considered to be a bad practice.<br>
      <br>
      Thanks,<br>
      <br>
      Sergey.<br>
      <br>
      <blockquote cite="mid:521B07A4.8020306@oracle.com" type="cite"> <br>
        <br>
        I will update my code according to most of your comments. Could
        you please reply to me asap if you think there's anything
        missing. Hopefully we can wrap things up within the week. <br>
        <br>
        <br>
        <br>
        - Charlie <br>
        <br>
        <br>
        <blockquote type="cite"> <br>
          On 22-Aug-13 22:43, Alex Buckley wrote: <br>
          <blockquote type="cite">I have no further comments. Thanks for
            all your work to test this new and expansive API. <br>
            <br>
            Alex <br>
            <br>
            On 8/20/2013 1:56 AM, Charlie Wang wrote: <br>
            <blockquote type="cite">Hi Alex, <br>
                 I used testInput.size() instead of static int clsIdx as
              suffix this <br>
              time. And other update according to your comments. <br>
              <br>
              Here's the link. Please review again. <br>
              <a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Epzhang/Charlie/TypeAnnotation1/webrev/">http://cr.openjdk.java.net/~pzhang/Charlie/TypeAnnotation1/webrev/</a>
              <br>
              <br>
              <br>
              - Charlie <br>
              <br>
              On 2013/8/9 1:05, Alex Buckley wrote: <br>
              <blockquote type="cite">- AnnotationTest.i/clsIdx: it
                wasn't the name of the variable I was <br>
                objecting to. It was the fact that you're relying on a
                mutable public <br>
                static variable at all. <br>
                <br>
                - GenTestCode should take Class&lt;? extends
                TestCaseGenerator&gt;, so you <br>
                don't need to cast to TestCaseGenerator in GenTestCode.
                (Same comment <br>
                for AnnotationTest.compileCode). <br>
                <br>
                - AnnotationTest.testInput is a Map&lt;Object,String&gt;
                but I'm sure you <br>
                can do better than Object for keys.' <br>
                <br>
                - ExecutableGetAnnotatedParameterTypesTest.testInput
                hides <br>
                AnnotationTest.testInput, but swaps the order of the
                type arguments! <br>
                Map&lt;String,Object&gt; is either wrong, or should not
                use Object. <br>
                <br>
                Alex <br>
              </blockquote>
            </blockquote>
          </blockquote>
          <br>
        </blockquote>
        <br>
      </blockquote>
      <br>
    </blockquote>
    <br>
  </body>
</html>