On Fri, 17 Nov 2023 15:44:52 GMT, Thomas Stuefe <stu...@openjdk.org> wrote:
> > > > > Can you please describe the problem you are trying to solve, and why > > > > > you think it is worth solving. You describe the thought process you > > > > > went through while doing the patch and possibly some other, older > > > > > patch. As it is, reading the PR text or the issue description is > > > > > confusing and requires reviewers to dig through comment threads on > > > > > older issues to parse it. > > > > > > > > > > > > I've changed to description for both to (hopefully) be more descriptive > > > > and clear. Sorry for the hassle, I'm not particularly good with words > > > > > > > > > Somewhat better, but not by much, sorry :-) > > > This patch is work to review, since it touches security-relevant stuff > > > and because mentally verifying that all the code paths you change are > > > equivalent to what happened before is real work. It is probably more work > > > than doing the patch. > > > Hence my question, what is the motivation, is there a bug that needs > > > fixing? Or is it just that you dislike these gotos? Is it just to get > > > Windows to build with GCC? Sorry, I did not follow your work, so I don't > > > have a context. > > > > > > This is a cleanup patch that follows the older one that enabled security to > > compile with permissive-, the problem with that one is that simply changing > > the locals to a split declaration and assignment is a bit of an ugly hack, > > since the locals then have undefined values when allocated. gcc already > > compiles fine with the old patch, this new one is not needed for either > > compiler (MSVC or gcc) to compile without errors. I felt like I had the > > responsibility to clean up what is essentially a hack just to get it to > > compile with permissive- on MSVC. This is also partially inspired by Phil's > > rejection of the old patch in #15096 :/ > > I don't have anything against gotos, nor do I dislike them :) > > Thank you for your explanation! I'll relegate to Phil for the actual review > then; I was just curious about the patch since it touched security-related > code. > > Cheers, Thomas I don't believe Phil does security reviews, that would be up to WeiJun and Valerie :P ------------- PR Comment: https://git.openjdk.org/jdk/pull/16682#issuecomment-1816687562
