<div dir="ltr">That's a good question. I guess it's a compromise.<div>From a backport-point-of-view, 1 PR (hence 1 JBS issue) per file is ideal. But that is not realistic of course, as it would clutter the JBS issues and the github PR's. </div><div>I think package-based PR's would be a good compromise. That would also make it feasible to review in depth.</div><div><br></div><div>Note that I think it would be better to change the title from "Eclipse warnings" into "Coding enhancements". I don't think we want Eclipse to decide the coding approaches in OpenJFX, as we can have follow-up PR's then about "NetBeans warnings" or "IntelliJ warnings" :)</div><div><br></div><div>- Johan</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Dec 6, 2023 at 5:06 PM Andy Goryachev <<a href="mailto:andy.goryachev@oracle.com">andy.goryachev@oracle.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="msg7054688798686111013">





<div lang="EN-US" style="overflow-wrap: break-word;">
<div class="m_7054688798686111013WordSection1">
<p class="MsoNormal"><span style="font-size:11pt;font-family:"Iosevka Fixed SS16"">Dear Johan:<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:"Iosevka Fixed SS16""><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:"Iosevka Fixed SS16"">What would be your best recommendation to minimize the burden?  Split into small PRs on a per-module or per-package basis?<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:"Iosevka Fixed SS16""><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:"Iosevka Fixed SS16"">Thanks,<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:"Iosevka Fixed SS16"">-andy<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:"Iosevka Fixed SS16""><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:"Iosevka Fixed SS16""><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:"Iosevka Fixed SS16""><u></u> <u></u></span></p>
<div id="m_7054688798686111013mail-editor-reference-message-container">
<div>
<div style="border-right:none;border-bottom:none;border-left:none;border-top:1pt solid rgb(181,196,223);padding:3pt 0in 0in">
<p class="MsoNormal" style="margin-bottom:12pt"><b><span style="font-size:12pt;color:black">From:
</span></b><span style="font-size:12pt;color:black">openjfx-dev <<a href="mailto:openjfx-dev-retn@openjdk.org" target="_blank">openjfx-dev-retn@openjdk.org</a>> on behalf of Johan Vos <<a href="mailto:johan.vos@gluonhq.com" target="_blank">johan.vos@gluonhq.com</a>><br>
<b>Date: </b>Monday, December 4, 2023 at 09:25<br>
<b>To: </b>Kevin Rushforth <<a href="mailto:kevin.rushforth@oracle.com" target="_blank">kevin.rushforth@oracle.com</a>><br>
<b>Cc: </b><a href="mailto:openjfx-dev@openjdk.org" target="_blank">openjfx-dev@openjdk.org</a> <<a href="mailto:openjfx-dev@openjdk.org" target="_blank">openjfx-dev@openjdk.org</a>><br>
<b>Subject: </b>Re: eclipse warnings<u></u><u></u></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:11pt">Also, these commits often affect many files at once (in scattered locations), and that makes backports harder.<u></u><u></u></span></p>
<div>
<p class="MsoNormal"><span style="font-size:11pt"><u></u> <u></u></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:11pt">- Johan<u></u><u></u></span></p>
</div>
</div>
<p class="MsoNormal"><span style="font-size:11pt"><u></u> <u></u></span></p>
<div>
<div>
<p class="MsoNormal"><span style="font-size:11pt">On Mon, Dec 4, 2023 at 6:14 PM Kevin Rushforth <<a href="mailto:kevin.rushforth@oracle.com" target="_blank">kevin.rushforth@oracle.com</a>> wrote:<u></u><u></u></span></p>
</div>
<blockquote style="border-top:none;border-right:none;border-bottom:none;border-left:1pt solid rgb(204,204,204);padding:0in 0in 0in 6pt;margin-left:4.8pt;margin-right:0in">
<div>
<p class="MsoNormal" style="margin-bottom:12pt"><span style="font-size:11pt">We did a few of these sort of cleanup fixes a year or so ago.<br>
<br>
In general, this sort of cleanup *might* be useful, but also causes some code churn and takes review cycles to ensure that there is no unintentional side effect.<br>
<br>
The last two might be OK cleanup tasks, but I wouldn't make them a high priority. Worth noting is that a seemingly redundant null check or instanceof check is not always a bad thing, so I wouldn't clean up all of them.<br>
<br>
The first group is the more interesting one. In some cases a potential null access can highlight actual bugs. However, I oppose any automated solution for these, since adding a null check where you don't expect a null (even if you IDE thinks it might be possible)
 can hide the root cause of a problem.<br>
<br>
We aren't going to enforce these, though, so you'll likely need to configure your IDE to be less picky.<br>
<br>
-- Kevin<br>
<br>
<u></u><u></u></span></p>
<div>
<p class="MsoNormal"><span style="font-size:11pt">On 12/4/2023 8:34 AM, Andy Goryachev wrote:<u></u><u></u></span></p>
</div>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<div>
<p class="MsoNormal"><span style="font-size:11pt;font-family:"Iosevka Fixed SS16"">Dear colleagues:</span><span style="font-size:11pt"><u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:"Iosevka Fixed SS16""> </span><span style="font-size:11pt"><u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:"Iosevka Fixed SS16"">Imported the openjfx project into another workspace with a more stringent error checking and discovered a few issues:</span><span style="font-size:11pt"><u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:"Iosevka Fixed SS16""> </span><span style="font-size:11pt"><u></u><u></u></span></p>
<ul type="disc">
<li class="MsoNormal">
<span style="font-size:11pt;font-family:"Iosevka Fixed SS16"">potential null pointer access: 295</span><span style="font-size:11pt"><u></u><u></u></span></li><li class="MsoNormal">
<span style="font-size:11pt;font-family:"Iosevka Fixed SS16"">unnecessary cast or instanceof: 190</span><span style="font-size:11pt"><u></u><u></u></span></li><li class="MsoNormal">
<span style="font-size:11pt;font-family:"Iosevka Fixed SS16"">redundant null check: 61</span><span style="font-size:11pt"><u></u><u></u></span></li></ul>
<p class="MsoNormal"><span style="font-size:11pt;font-family:"Iosevka Fixed SS16""> </span><span style="font-size:11pt"><u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:"Iosevka Fixed SS16"">Do we want to clean these up?</span><span style="font-size:11pt"><u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:"Iosevka Fixed SS16""> </span><span style="font-size:11pt"><u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:"Iosevka Fixed SS16"">-andy</span><span style="font-size:11pt"><u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:"Iosevka Fixed SS16""> </span><span style="font-size:11pt"><u></u><u></u></span></p>
</div>
</blockquote>
<p class="MsoNormal"><span style="font-size:11pt"><u></u> <u></u></span></p>
</div>
</blockquote>
</div>
</div>
</div>
</div>
</div>

</div></blockquote></div>