RFR: 8160404: RelocationHolder constructors have bugs [v2]

Kim Barrett kbarrett at openjdk.org
Fri Dec 16 02:05:06 UTC 2022


On Thu, 15 Dec 2022 12:28:29 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

>> Kim Barrett has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   blank lines in include blocks
>
> src/hotspot/share/code/relocInfo.hpp line 528:
> 
>> 526:     Relocation* reloc = ctor(_relocbuf);
>> 527:     check_reloc_placement(reloc);
>> 528:   }
> 
> It seems that the code is like this, rather than using `typename T, typename...Args` (where `T` is the relocation type), because it is not possible to specify explicit type arguments when calling a constructor template? (So you can't specify `T`).
> 
> But, you could truck in the type argument on `Construct`, and have it be inferred. So, this could become:
> 
> Suggestion:
> 
>   template<typename T>
>   struct Construct {};          // Tag for selecting this constructor.
>   template<typename T, typename... Args> RelocationHolder(Construct<T>, const Args&...args) {
>     check_reloc_type<T>();
>     Relocation* reloc = ::new (_relocbuf) T(args...);
>     check_reloc_placement(reloc);
>   }
> 
> Callers provide e.g. `Construct<T>()` in `construct` or `Construct<Relocation>()` in the default constructor. `construct` can just forward the rest of the args.
> 
> This seems a little simpler (by avoiding the use of lambdas, and making it more locally obvious that the constructor is doing a placement new into `_relocbuf`), so maybe this is preferable?

Yes, that works, and it's an improvement. Thanks. It also let me merge most of
the implementation of that constructor with the implementation of
`copy_into_impl`, which now use new `emplace_resource`.

I also took a moment to use `alignas` instead of a union, since JDK-8297912
was integrated earlier today.

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

PR: https://git.openjdk.org/jdk/pull/11618


More information about the hotspot-compiler-dev mailing list