Investigate public definitions of self() in compiler

[mstoodle]

@georgkrylov pointed out in #4557 that OMRCompilation.hpp defines self() as a public member function. I believe the nature of self() should be constrained to protected access: there should not be a need for a class outside of the extensible hierarchy of e.g. Compilation to call Compilation's self() function. External classes should only referring to the fully extended TR::Compilation type.

See #4557 (comment)

which includes this code snippet from OMRCompilation.hpp :
https://github.com/eclipse/omr/blob/7b4155a69859e4422f742acbffe712d1d15ee4cc/compiler/compile/OMRCompilation.hpp#L288-L312

[AAbhiJ]

hi @mstoodle @0xdaryl @fjeremic
i am new here can i take this issue

[mstoodle]

That's great, @ABHI12800 ! Thank you for offering to help. Is the context for this item clear to you, or is there anything we can do to help you get started?

fwiw, I recommend you start with just one (one of the simpler) classes that has public self() references and create one commit in a PR for us to review, just to make sure you're on the right track before you spend time modifying a bunch of classes.

[AAbhiJ]

fwiw, I recommend you start with just one (one of the simpler) classes that has public self() references and create one commit in a PR for us to review, just to make sure you're on the right track before you spend time modifying a bunch of classes.

Thanks for replying @mstoodle. Sure, I will start with one.

Thank you for offering to help. Is the context for this item clear to you, or is there anything we can do to help you get started?

I think the context is little bit clear to me. And I will surely need help since its my first contribution.

[mstoodle]

You'll probably want to read up on extensible classes a bit, which you can find in docs/compiler/extensible_classes [1]. In particular, you'll want to read the section on self() [2] . We use extensible class hierarchies to compose together code from OMR and consuming projects. Extensible classes are an alternative to CRTP (the Curiously Recurring Template Pattern [3]) that we see as more attractive for our particular use case.

Just like when using CRTP, however, we need to cast this to the most derived class when composing . To avoid seeing that cast all over the place in extensible classes, we define a self() function to be used where ever this is used (either explicitly or implicitly).

[1] https://github.com/eclipse/omr/blob/master/doc/compiler/extensible_classes/Extensible_Classes.md
[2] https://github.com/eclipse/omr/blob/master/doc/compiler/extensible_classes/Extensible_Classes.md#self
[3] https://en.wikipedia.org/wiki/Curiously_recurring_template_pattern

[AAbhiJ]

hey I have made the changes to one class OMRCompilation.hpp and removed the declaration from public and rewrite it using protected can you check it, is this is the right way or I should create this commit in PR.

[mstoodle]

That will do, yes. Another way would be to move the self() declaration down to the already existing protected: section of the class. But I think either one is fine. @0xdaryl do you have any preference one way or the other?

Also, just for convenience, @ABHI12800 has opened a PR for review here #5793 .

[0xdaryl]

I don't have any real preference. There are lots of different styles in use throughout the code base. Thanks for taking on these changes.

[AAbhiJ]

Thanks for your reply. :)
So should I make changes to other files?
and I will check the Bot details and make the necessary changes.

[mstoodle]

I suggest we get your first PR merged as you work out all the mechanics involved in contributing, then we would happily welcome more changes like this one from you.

[AAbhiJ]

Opened PR for OMR::AliasBuilder #5799
Opened PR for OMR::Method #5801
Opened PR for OMR::SymbolReferenceTable.hpp #5802

[AAbhiJ]

Is this ok?

[fjeremic]

@ABHI12800 those PRs are unnecessarily small. May I suggest we open up one PR and have all the commits in there? Having multiple PRs requires multiple rounds of testing and eyes from committers.

[AAbhiJ]

Sorry ,I will do that.

[AAbhiJ]

Opened PR as #5803

[georgkrylov]

Hello everyone, to assist the community, another public definition of self():

TR::MemoryReference *self();