On Mon, 14 Aug 2023 20:57:24 GMT, Phil Race <p...@openjdk.org> wrote:
> I have no time to look at the client changes for quite some time so do not > push it. No matter how many other people approve it. And in the meantime you > can (1) explain how many client tests you ran - and it had better be all of > them :-) (2) add a comment in the PR to explain each client change you made - > at the site of the change. Comments like "Fields in awt_TextComponent.cpp" > are not an explanation (3) On the broader front, I note that you just start > off with "We should set the -permissive- flag for the Microsoft Visual C > compiler" without explaining WHY and what it does. In fact from what I've > read about -it seems like we would not want it .. it downgrades errors to > warnings .. and surely that's a bad thing ? No worries, I was not planning on committing without approval from each group, this was why I set the required reviewers to 5, 2 for HotSpot, one for build, and so on. I'll add the comments later on, but right now I can explain the changes directly since it's slightly easier to do so: The changes to awt involve enclosing several areas which make heavy use of goto in scopes to avoid them jumping over the initialization of locals, reordering the order of includes to avoid Microsoft specific headers from using our redefined malloc, moving the definition of C++ static fields outside of extern "C" blocks, and removing the class qualifiers from field declarations in several classes. The last one is fairly obvious - It isn't legal C++ grammar to do so. For the fields that were moved, they have C++ linkage initially by virtue of being static members of a class, but their definitions are inside extern "C" blocks, which causes their linkage to conflict between C++ and C. For the reordering of includes, see my conversation with Thomas - With permissive- enabled C++ templates are properly conforming, and the header comip.h (which is included by comdef.h) uses a templated malloc somewhere, which due to our #define malloc in alloc.h fails since the identifier is obviously not d efined. This is actually a major problem that is potentially causing an invisible bug in awt.dll somewhere, aside from it not compiling with permissive-, the redefining of malloc and all other allocators like this is something @tstuefe has expressed some concern over, but I don't really know how to deal with the bigger underlying problem outside of reordering the includes to not let comdef.h use our #define'd malloc. For the enclosing of gotos in scopes Thomas has suggested using RAII instead of labels in awt, which is something that could be done outside of this change, but I'm still figuring out if doing so for the sites that this is a problem for in this change would be neater Noted, I'll also improve the explaination on what permissive- does and why we want it in just a moment ------------- PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-1678344486
