Basic implementation of the feature has been done. However, now 30 tests fail with errors similar to

@filiphr Could you please conduct a review and help me to understand how to fix those tests?
It seems we should add tests for testing the generated code for JDK 19+

Thanks a lot @Obolrom. Looks way way cleaner now. I'll have a look for the fixtures to work on multiple java version

@filiphr @Obolrom Sorry for the late reply.

I have been working on the review too.

My main concern about the provided code is that, in my opinion, it actually doesn't deal with the actual problem. Please, let me trying explaining myself.

The problem the new factory methods trying addressing is the following.

Let's say you need to allocate N elements.

When you create an ArrayList, for instance, you do something like the following:

List<String> list = new ArrayList<>(N);

The JVM will ensure that enough room for the N elements will be allocated.

In contrast, if you do the same with a HashMap, for example:

Map<String,Object> map = new HashMap<>(N);

and you actually need N elements to be allocated, you are in trouble, because in fact the JVM will allocate N*<load factor>, by default, 0.75, elements instead: i.e., if you push N elements actually to that HashMap it will be internally rehashed.

Addressing this issue is the reason behind the new factory methods. Please, see the actual code implementation.

In other words, I think the important point here is the actual initial capacity and not the load factor, as the implementation proposed suggests, as the implementation is focused on.

What do you think?

@jccampanero, sorry but I don't think that I understand your comment. The fact that Map<String,Object> map = new HashMap<>(N); does not load the right amount of data is something that we already know and that we handle in MapStruct.

The current implementation looks like:

Map<String,Object> map = new HashMap<>( Math.max( (int) ( N / .75f ) + 1, 16 );

The goal of this PR is to generate code (on Java 19 or later) that looks like:

Map<String,Object> map = HashMap.newHashMap( N );

With this PR, based on the java version of the code we would either use the load factor or the new factory method.

Hi @filiphr, thank you very much for your feedback.

I absolutely agree with you. I was merely trying to provide some context, but I understand the purpose of the PR. I apologize if I didn't explain myself clearly earlier.

First, I want to say that the provided implementation is quite good. Thank you, @Obolrom.

However, I believe it is very focused on the load factor when we should be concentrating more on the factory method itself. I have performed a code review which I hope clarifies my point.

Thank you very much for your patience, @filiphr, and great job, @Obolrom.

I'll have a look for the fixtures to work on multiple java version

@filiphr any updates about supporting fixtures for multiple java versions?

@jccampanero I understood your comments now. @Obolrom I did some polishing from the feedback provided by @jccampanero. Let me know if you are OK with this.

I also gave it a go to have java 21 based fixtures. I'm not super happy with the solution and it doesn't work well with the current version of the eclipse plugin for some reason.

@filiphr code looks a little bit cleaner now.
Regarding the Eclipse plugin. I think the problem is with the hardcoded compiler max source version.
Please check EclipseCompilingExtension.getSourceVersion() method

@filiphr, I did the fix for the Eclipse plugin. Please, share your thoughts how we can improve this solution.
@jccampanero, what do you think?

@filiphr could you please conduct a code review? It seems we are close to merge this PR 😊

@filiphr, I have rebased the branch and fixed a test
@thunderhook, could you please take a look at the PR?

@thunderhook, could you please take a look at the PR?

I'm sorry, I'm on holiday at the moment. Maybe I can make it in about 2 weeks if Filip hasn't had a look yet.

@filiphr , it seems the PR in status 'changes requested'. However, those changes are outdated.
Or have I missed something?

@filiphr , it seems the PR in status 'changes requested'. However, those changes are outdated.
Or have I missed something?

It is like that, because it still looks at my old review. I'll try to find some time to review this. Sorry that it is taking longer @Obolrom

@filiphr @thunderhook
I have updated the branch, it is up to date now.

Thanks @Obolrom. I'll try to review it it this week during my holidays.

I finally got the chance to merge this. Thanks @Obolrom. I did some final polishing in e34abec

@filiphr thank you a lot