Panama source code fails to compile

[Ty Young]

On 1/4/21 3:21 PM, Maurizio Cimadamore wrote:
>
>
> On 04/01/2021 19:38, Ty Young wrote:
>>>>
>>>>
>>>> - Padding layout should probably be a subclass of ValueLayout and 
>>>> just override methods as needed. Having every memory layout have a 
>>>> isPadding() method doesn't make much sense. The PaddingLayout class 
>>>> can then be moved to an internal package as there is no way to 
>>>> create an instance outside of ofPaddingBits as an API user. The 
>>>> same could be done for AbstractLayout.
>>> Are you talking about API or impl? Impl-wise, we could resort to 
>>> overriding for isPadding, but if PaddingLayout is to be 
>>> impl-private, then user has no way to do instanceof PaddingLayout, 
>>> so you need a predicate?
>>
>>
>> Bit of both. ofValueBits can be modified to return a ValueLayout 
>> since, under this change, PaddingLayout extends ValueLayout and 
>> "is-a" ValueLayout. isPadding() will then be made exclusive to 
>> ValueLayout and would be how you determine if a ValueLayout is 
>> padding. If that method exists, I don't see a reason why exposing 
>> PaddingLayout as part of public API and letting a user do instanceof 
>> is needed as isPadding() exists. isPadding() should maybe use 
>> getClass() instead of instanceof, don't think it's a big deal but 
>> it's slightly faster IIRC. There isn't any reason to expose 
>> PaddingLayout or AbstractLayout in public API AFAIK.
>
> Gotcha.
>
> Yes, in principle padding can be a special case of value - except then 
> you have endianness thrown in as well which isn't horrible, but 
> doesn't make a lot of sense either.
>
> My mental model is more:
>
> LeafLayout
> ValueLayout <: LeafLayout
> PaddingLayout <: LeafLayout
>
> which then led to the current compressed hierarchy.
>

The current API doesn't do that anyway, ofPaddingBits() returns a 
MemoryLayout which is ambiguous in terms of implementation. Sure, in 
most cases people just make as many ofPaddingBits() calls as needed 
directly in a struct's layout so it doesn't matter, but if someone were 
to hypothetically store the returned MemoryLayout in a variable there 
would be no type information to indicate it was padding(this is already 
true for MemoryLayouts padding constants, actually). Someone could use a 
ValueLayout to pad the struct all the same.


How about keeping PaddingLayout where it is, having ofPaddingLayout() 
return PaddingLayout, and remove isPadding() for every layout type? This 
way the type is clear and people can do instanceof and/or getClass() 
themselves ? Would you accept a pull request for that?



>>
>>
>>>>
>>>>
>>>> - GroupLayout should probably be split into StructLayout and 
>>>> UnionLayout. This is the biggest change, but it simplifies the 
>>>> code, provides better clarity on what's actually being represented, 
>>>> and allows for future struct/union specific operations to be added 
>>>> without adding a bunch of if(!isUnion) throw new 
>>>> UnsupportedOperationException(); type code.
>>>
>>> Less sure about this.
>>>
>>> I think pattern matching will also help a great deal with this, at 
>>> some point.
>>
>>
>> My understanding is that you need a type to do pattern matching? The 
>> Kind enum used internally for isStruct() and isUnion() is private.
>
> For now you need a type, yes.
>
> In the future, when pattern declaration will be available, we can add 
> an extractor for each static factory there is in the API.
>
> if (layout instanceof GroupLayout.ofStruct(layoutA, layoutB)) {
>   ...
> }
>
> Something like that.
>

So abandoning long standing and existing good coding/API design 
practice(SOLID 1: Single Responsibility) in the now because someday in 
the future(probably 2+ years from now, I'm guessing?) we'll have a shiny 
new language feature that will let you do what you can do now if the API 
was improved? Not only are you potentially backing yourselves into a 
corner API design wise, you're forcing people to use language features 
they might not want to use.


Is any of this pattern matching stuff going to be performant anyway? 
Like, an if statement comparing class pointers is faster than doing the 
code above, right?


> Maurizio
>
>>
>>
>> If the issue is unifying the types then GroupLayout could maybe be 
>> repurposed so that it simply exposes memberLayouts() and other shared 
>> code?
>>