RFR: 7903608: Cyclic initialization leads to NPE in header class with global variable [v2]

[Jorn Vernee]

On Thu, 14 Dec 2023 15:01:22 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> src/main/java/org/openjdk/jextract/impl/SourceFileBuilder.java line 95:
>> 
>>> 93: 
>>> 94:     public JavaFileObject toFile(String suffix, Function<String, String> finisher) {
>>> 95:         return Utils.fileFromString(packageName, STR."\{className}\{suffix}", finisher.apply(sb.toString()));
>> 
>> So, it looks like we adjust the class name at the last moment when generating the file. That's fine I think, but it also means that the `SourceFileBuilder::className` field is 'fake'/a place holder in some cases. That seems like it might be problematic if other code starts to naively use `SourceFileBuilder::className`.
>> 
>> Could we maybe make this more explicit? e.g. could we make the class name an `Optional<String>` ? and then require the name to be injected when calling `toFile`? Or maybe always require the class name to be specified to `toFile`, and just remove the `className` field?
>
>> So, it looks like we adjust the class name at the last moment when generating the file. That's fine I think, but it also means that the `SourceFileBuilder::className` field is 'fake'/a place holder in some cases. That seems like it might be problematic if other code starts to naively use `SourceFileBuilder::className`.
> 
> Yep - I was aware of this, and probably the biggest reason as to why I wasn't proud with the change. I looked at the usages of className the other day, and they felt a bit too many - and depended on in various places when we generate code. I can take another look, but I was in doubt as to whether fix this issue first and maybe improve the code later (given that jextract is currently quite broken). What do you think?

We can address this later as well.

-------------

PR Review Comment: https://git.openjdk.org/jextract/pull/160#discussion_r1426855716