Page MenuHomePhabricator

[ELF] - Linkerscript: simplify access to templated methods from parser.
ClosedPublic

Authored by grimar on Aug 25 2016, 7:25 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar updated this revision to Diff 69243.Aug 25 2016, 7:25 AM
grimar retitled this revision from to [ELF] - Linkerscript: simplify access to templated methods from parser..
grimar updated this object.
grimar added a reviewer: ruiu.
grimar added subscribers: llvm-commits, davide, grimar, evgeny777.
rafael added inline comments.
ELF/LinkerScript.h
140 ↗(On Diff #69243)

This is the same as Script<ELFT>::X, just with a non templated type, no?

Can you rename ScriptMethods to LinkerScriptBase and replace Script<ELFT>::X with a "LinkerScriptBase *Script"?

ruiu edited edge metadata.Aug 30 2016, 11:21 AM

As I wrote in the comment to the other patch, I think I still prefer the current way than the way you are suggesting in this patch. The current approach seems silly but simple.

grimar updated this revision to Diff 69822.Aug 31 2016, 3:38 AM
grimar edited edge metadata.
  • Rebased.
  • Addressed comments partially.
In D23872#529252, @ruiu wrote:

As I wrote in the comment to the other patch, I think I still prefer the current way than the way you are suggesting in this patch. The current approach seems silly but simple.

I see. But there are 4 methods already and probably we`ll need more. Each one use of non-template stuff is an excessive call redirection and always a new method with bunch of code.
Virtual calls for linkerscript tasks costs nothing at the same time.

I updated this patch just in case to show how much code is removed from head revision.

ELF/LinkerScript.h
140 ↗(On Diff #69243)

Not sure I understand the idea correctly. If I replace usings of Script<ELFT>::X with a "LinkerScriptBase *Script",
I`ll need to add more virtual methods to the LinkerScriptBase. And that is not possible for all of them, because few
are templated:

std::vector<PhdrEntry<ELFT>> createPhdrs();
bool shouldKeep(InputSectionBase<ELFT> *S);

I tried to do:

extern LinkerScriptBase* ScriptBase;
template <class ELFT> LinkerScript<ELFT> *getScript() {
  return static_cast<LinkerScript<ELFT> *>(ScriptBase);
}

Instead of:

template <class ELFT> struct Script { static LinkerScript<ELFT> *X; };
template <class ELFT> LinkerScript<ELFT> *Script<ELFT>::X;

But use of getScript<ELFT() everywhere in the code introduces lots of minor changes,
not sure it looks nicer either.

I ended up with adding single global variable.

rafael accepted this revision.Aug 31 2016, 10:43 AM
rafael added a reviewer: rafael.

Something like getScript is what I had in mind, but the current solution is OK too.

I like this change, but wait to see if Rui is OK with it too.

ELF/LinkerScript.cpp
629 ↗(On Diff #69822)

Is this being used?

This revision is now accepted and ready to land.Aug 31 2016, 10:43 AM
ruiu added a comment.Sep 6 2016, 6:30 PM

I'm trying to not do too much stuff in the "object-oriented" style, but this seems to be undeniably an improvement, so we probably should go with this.

ELF/LinkerScript.cpp
44 ↗(On Diff #69822)

Format.

ELF/LinkerScript.h
192–193 ↗(On Diff #69822)

This comment is for struct Script and Script<ELFT>::X, so you shouldn't add the new line below this.

194 ↗(On Diff #69822)

clang-format.

grimar updated this revision to Diff 70511.Sep 7 2016, 1:38 AM
grimar edited edge metadata.
  • Addressed review comments.
ruiu accepted this revision.Sep 7 2016, 3:25 PM
ruiu edited edge metadata.

LGTM with this fix.

ELF/LinkerScript.h
205 ↗(On Diff #70511)

Please run clang-format on this patch. You want to put a space before * instead of after it.

grimar added inline comments.Sep 8 2016, 1:05 AM
ELF/LinkerScript.h
205 ↗(On Diff #70511)

Sorry for this.

This revision was automatically updated to reflect the committed changes.