<html>
  <head>
    <meta content="text/html; charset=utf-8" http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    <div class="moz-cite-prefix">On 29.07.2016 17:30, Philip Race wrote:<br>
    </div>
    <blockquote cite="mid:579B6885.6070706@oracle.com" type="cite">
      <meta content="text/html; charset=utf-8" http-equiv="Content-Type">
      <br>
      <br>
      On 7/29/16, 7:00 AM, Vadim Pakhnushev wrote:
      <blockquote
        cite="mid:4e04aa2a-da69-7983-2644-c1e7a5d23af3@oracle.com"
        type="cite">
        <meta content="text/html; charset=utf-8"
          http-equiv="Content-Type">
        <div class="moz-cite-prefix">On 29.07.2016 16:28, Philip Race
          wrote:<br>
        </div>
        <blockquote cite="mid:579B5A03.7000801@oracle.com" type="cite">
          <meta content="text/html; charset=utf-8"
            http-equiv="Content-Type">
          <br>
          <br>
          On 7/29/16, 3:23 AM, Vadim Pakhnushev wrote:
          <blockquote
            cite="mid:8ef6de35-011d-5218-3bdd-f3547e65ee29@oracle.com"
            type="cite">
            <meta content="text/html; charset=utf-8"
              http-equiv="Content-Type">
            <div class="moz-cite-prefix">Phil,<br>
              <br>
              I guess you wanted to remove the lines in the
              Awt2dLibraries.gmk?<br>
            </div>
          </blockquote>
          <br>
          Ah, yes. Not just disable them<br>
          <blockquote
            cite="mid:8ef6de35-011d-5218-3bdd-f3547e65ee29@oracle.com"
            type="cite">
            <div class="moz-cite-prefix"> <br>
              Do you think it's worth it to rewrite these assignments as
              separate assignment and a condition?<br>
              Especially long ones with a lot of parentheses?<br>
              Like this one, instead of<br>
              if ((j = ((mlib_s32) ((mlib_addr) psrc_row & 4)
              >> 2))) {<br>
              <br>
              j = (mlib_s32) ((mlib_addr) psrc_row & 4) >> 2;<br>
              if (j != 0) {<br>
            </div>
          </blockquote>
          <br>
          I don't know. Where would I stop ?<br>
        </blockquote>
        <br>
        Where it doesn't feel weird anymore?<br>
        For example, is this line correct?<br>
              if (j = (((mlib_addr) pdst_row & 2) != 0)) {<br>
        It seems very weird for me that we assign a boolean value to the
        loop variable.<br>
        It looks like there's an error in parentheses and it should
        instead look like:<br>
              if ((j = (((mlib_addr) pdst_row & 2) != 0) {<br>
        Yeah, in this particular case it doesn't even matter but still
        assignments in conditions can very easily lead to errors.<br>
      </blockquote>
      <br>
      OK I will take a *limited* look at this.<br>
    </blockquote>
    <br>
    Yeah it's just 2 identical lines in the mlib_c_ImageCopy.c<br>
    <br>
    <blockquote cite="mid:579B6885.6070706@oracle.com" type="cite">
      <blockquote
        cite="mid:4e04aa2a-da69-7983-2644-c1e7a5d23af3@oracle.com"
        type="cite"> <br>
        <blockquote cite="mid:579B5A03.7000801@oracle.com" type="cite">
          <br>
          <blockquote
            cite="mid:8ef6de35-011d-5218-3bdd-f3547e65ee29@oracle.com"
            type="cite">
            <div class="moz-cite-prefix"> <br>
              Also seeing macro calls without a semicolon is weird.<br>
              I don't see any need in parentheses in the definition of
              FREE_AND_RETURN_STATUS so maybe it's possible to rewrite
              it without trailing semicolon?<br>
            </div>
          </blockquote>
          <br>
          I anticipated the question and it is already addressed in my
          last<br>
          paragraph right at the very bottom of the review request.<br>
        </blockquote>
        <br>
        Oops, missed that.<br>
        <br>
        <blockquote cite="mid:579B5A03.7000801@oracle.com" type="cite">
          Basically that pattern has an "if (x==NULL) return". If you
          don't<br>
          have that "if" then the compiler would have objected to that
          too !<br>
        </blockquote>
        <br>
        I'm not sure I undestand this.<br>
      </blockquote>
      <br>
      I mean I  without the condition the compiler can tell you *never*
      reach<br>
      the while (0) and so objected on those grounds. I tried this.<br>
    </blockquote>
    <br>
    Got it.<br>
    <br>
    <blockquote cite="mid:579B6885.6070706@oracle.com" type="cite">
      <blockquote
        cite="mid:4e04aa2a-da69-7983-2644-c1e7a5d23af3@oracle.com"
        type="cite"> <br>
        I mean why not rewrite the macro like this:<br>
        #define FREE_AND_RETURN_STATUS \<br>
        if (pbuff != buff) mlib_free(pbuff); \<br>
        if (k != akernel) mlib_free(k); \<br>
        return status<br>
        #endif /* FREE_AND_RETURN_STATUS */<br>
        <br>
        Yes it's prone to errors like if (foo) FREE_AND_RETURN_STATUS;
        but all its usages are separate statements.<br>
      </blockquote>
      <br>
      hmm .. I suppose could .. but not pretty either.<br>
      Also if going that route it could be<br>
      <br>
      #define FREE_AND_RETURN_STATUS \<br>
      { \<br>
      if (pbuff != buff) mlib_free(pbuff); \<br>
      if (k != akernel) mlib_free(k); \<br>
      } \<br>
      return status<br>
      #endif /* FREE_AND_RETURN_STATUS */<br>
      <br>
      ??<br>
    </blockquote>
    <br>
    What's the point of parentheses here?<br>
    I thought that the whole point of curly braces and do .... while(0)
    stuff was to enable the macro calls in conditions like if (foo)
    CALL_MACRO; without the curly braces around CALL_MACRO.<br>
    But if you put curly braces around only part of the macro definition
    this would be broken anyway.<br>
    <br>
    Vadim<br>
    <br>
    <blockquote cite="mid:579B6885.6070706@oracle.com" type="cite"> <br>
      -phil.<br>
      <blockquote
        cite="mid:4e04aa2a-da69-7983-2644-c1e7a5d23af3@oracle.com"
        type="cite"> <br>
        Vadim<br>
        <br>
        <blockquote cite="mid:579B5A03.7000801@oracle.com" type="cite">
          <blockquote
            cite="mid:8ef6de35-011d-5218-3bdd-f3547e65ee29@oracle.com"
            type="cite">
            <div class="moz-cite-prefix"> <br>
              Thanks,<br>
              Vadim<br>
              <br>
              On 29.07.2016 2:31, Philip Race wrote:<br>
            </div>
            <blockquote cite="mid:579A95D6.3090604@oracle.com"
              type="cite">
              <meta http-equiv="content-type" content="text/html;
                charset=utf-8">
              <meta http-equiv="content-type" content="text/html;
                charset=utf-8">
              Bug: <a moz-do-not-send="true"
                class="moz-txt-link-freetext"
                href="https://bugs.openjdk.java.net/browse/JDK-8074843">https://bugs.openjdk.java.net/browse/JDK-8074843</a><br>
              Fix: <a moz-do-not-send="true"
                class="moz-txt-link-freetext"
                href="http://cr.openjdk.java.net/%7Eprr/8074843/">http://cr.openjdk.java.net/~prr/8074843/</a><br>
              <br>
              Here's a sampling of the warnings that I think covers
              most, maybe all, of the cases<br>
              LINUX<br>
              mlib_ImageAffine_NN_Bit.c:87:81: error: suggest
              parentheses around '-' in operand of '&'
              [-Werror=parentheses]<br>
                       res = (res & ~(1 << bit)) |
              (((srcPixelPtr[X >> (MLIB_SHIFT + 3)] >> (7 -
              (X >> MLIB_SHIFT) & 7)) & 1) <<<br>
                                                                                
              ^<br>
              mlib_ImageAffine_NN_Bit.c:153:81: error: suggest
              parentheses around '-' in operand of '&'
              [-Werror=parentheses]<br>
                       res = (res & ~(1 << bit)) |
              (((srcPixelPtr[X >> (MLIB_SHIFT + 3)] >> (7 -
              (X >> MLIB_SHIFT) & 7)) & 1) << bit);<br>
              <br>
              -----------------<br>
              mlib_c_ImageCopy.c: In function 'mlib_c_ImageCopy_s16':<br>
              mlib_c_ImageCopy.c:439:5: error: suggest parentheses
              around assignment used as truth value
              [-Werror=parentheses]<br>
                   STRIP(pdst, psrc, src_width, src_height, mlib_u16);<br>
                   ^<br>
              -<br>
              MAC ...<br>
              <br>
              mlib_c_ImageCopy.c:331:5: error: using the result of an
              assignment as a condition without parentheses
              [-Werror,-Wparentheses]<br>
                  STRIP(pdst, psrc, src_width, src_height, mlib_u8);<br>
                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~<br>
              mlib_c_ImageCopy.c:185:11: note: expanded from macro
              'STRIP'<br>
                  if (j = w &
              1)                                              \<br>
                      ~~^~~~~~~<br>
              mlib_c_ImageCopy.c:331:5: note: place parentheses around
              the assignment to silence this warning\<br>
              <br>
              ---<br>
              <br>
              <br>
              ---<br>
              SOLARIS<br>
              mlib_ImageConv_16ext.c", line 532: statement not reached
              (E_STATEMENT_NOT_REACHED) <br>
              <br>
              This last one was not nice just the ";" was considered a
              statement<br>
              after the {XX; YY; return Z} macro expansion<br>
              and turning into "do { {....} } while (0)" did not  help
              since<br>
              then it said "loop terminator not reached - other cases we
              have<br>
              like this have at least a condition in the macro.<br>
              <br>
              -phil.<br>
              <span class="overlay-icon aui-icon aui-icon-small
                aui-iconfont-edit"></span> </blockquote>
            <br>
          </blockquote>
        </blockquote>
        <br>
      </blockquote>
    </blockquote>
    <br>
  </body>
</html>