This is an archive of the discontinued LLVM Phabricator instance.

ELF: Move initSymbols to Symbols.cpp.
AbandonedPublic

Authored by ruiu on Mar 13 2016, 1:56 PM.

Details

Reviewers
grimar
Summary

This effectively rolls back r263133. I think moving this code to
Driver was a violation of modularization, and it felt odd to me that
driver was manipulating the details of the internal symbols.

Diff Detail

Event Timeline

ruiu updated this revision to Diff 50557.Mar 13 2016, 1:56 PM
ruiu retitled this revision from to ELF: Move initSymbols to Symbols.cpp..
ruiu updated this object.
ruiu added a reviewer: grimar.
ruiu added a subscriber: llvm-commits.
grimar edited edge metadata.EditedMar 14 2016, 12:53 AM

Ok, but why don't you want to stay on initial version http://reviews.llvm.org/D18047 ?
That keeps initSymbols() in symbols.cpp, but does not initialize them for each ELFT.

May be revert of r263214 is enough here ?

ruiu added a comment.Mar 14 2016, 10:41 AM

Sorry, I replied to a different thread.

So, it's ecause that Symbol.cpp needs to initialize symbols for each ELF type is an internal detail of the file. initSymbol, or in general, init<filename> is the initializer function for a file. Callers of the initializer should not take care of the details whether the function works on ELFT-specialized objects or not.

I would probably hold this until we need a second initSomething function.

Cheers,
Rafael

In D18135#374658, @ruiu wrote:

Callers of the initializer should not take care of the details whether the function works on ELFT-specialized objects or not.

That is arguable. Here it do excessive work when initializing something that is not used. That has no impact on perfomance but actually confuses, makes code reader to think that these symbols are required somehow when them are not.

I would agree with Rafael here to hold it if possible until we see any other initXXX function.

If you don't plan to commit this - please abandon.

ruiu abandoned this revision.Mar 16 2016, 1:47 PM

I'm not convinced that that is not worth it, but no reason to push this strongly, so I'm okay to abandon it for now.