[lworld] RFR: 8247357: Flattenable field concept needs some cleanup

[Frederic Parain]

John,

Thank you for looking at these changes.

“is_inline” might be confusing in the sense that it can be interpreted
as a property of the field layout. And “is_declared_inline” shares the
same issue (could be interpreter as a field modifier).
What the is_inline() methods really do, is to answer the question: 
is the type of this field an inline type? So, it’s a type question,
and not a layout question. And sometimes, we use is_inline to perform
checks that are not related to the layout, but to the properties of
the type (like null-freeness).

To prevent the confusion, I would propose to change “is_inline” to
“is_inline_type”, so the it would be obvious that the test is about
the type of the field.

And to have similar names, we would follow your suggestion and
rename “is_flattened” to “is_allocated_inlined"

So: 
	if(fd->is_inline_type()) {        // -> clearly a type test

and
        if(fd->is_allocated_inline()) {   // -> clearly a layout test


Would these new names address the concerns you have?

Regards,

Fred



> On Jun 10, 2020, at 15:37, John Rose <john.r.rose at oracle.com> wrote:
> 
> [consolidate and resend after name fix]
> 
> On Jun 10, 2020, at 11:55 AM, Frederic Parain <fparain at openjdk.java.net> wrote:
>> 
>> Please review these changes cleaning up the flattenable field concept.
>> The concept has evolved with time and now all fields with an inline type are by definition flattenable, so the need to
>> have a "flattenable bit" on the side is gone. The changeset contains a mix of renaming and code cleaning. The changes
>> don't include JIT code, which would be fix in a follow-up patch.
> 
> I’m glad to see occurrences of “value” go away in favor of “inline”.
> 
> (The language gurus won’t absolutely promise that “inline” is the
> final word on terminology, but I think at least for the JVM code,
> “inline” is a far more descriptive term than “value”.)
> 
> The “is_flattenable” bit was often accompanied by an “is_flattened”
> bit which reported whether the intended flattening actually took place.
> Having their names be similar helped make the code understandable.
> So I suggest renaming those guys also, since you are touching all
> that code now.
> 
> The term “is_inline” is ambiguous when reading the code.  Where there
> is any doubt about whether it means “is intended to be inlined” vs.
> “is actually inlined in the layout”, the term should be made more explicit.
> 
> So I suggest:
> 
> s/is_flattenable/is_declared_inline/
> s/is_flattened/is_allocated_inline/
> 
> Maybe that’s overkill?  But I think just “is_inline” is not clear enough.
> 
> — John
> 
> P.S. To be clear:  I’m not suggesting that systematically, just where
> the distinction exists between “could be” and “actually is” flattened.
> Names like STATIC_INLINE in the CFP are completely unambiguous;
> they shouldn’t be STATIC_ALLOCATED_INLINE or the like.
> 
> In the CFP, “has_flattenable_fields” could go either way, but I think
> “has_declared_inline_fields” would be safer.  It’s not clear whether it
> means “my definer has declared inline fields in me”, or “I actually
> flattened one or more of my fields”.