This is an archive of the discontinued LLVM Phabricator instance.

Convert option tables to use llvm::ArrayRefs
ClosedPublic

Authored by zturner on Sep 22 2016, 10:21 AM.

Details

Summary

This change is very mechanical. All it does is change the signature of Options::GetDefinitions() and OptionGroup::GetDefinitions() to return an llvm::ArrayRef<OptionDefinition> instead of a const OptionDefinition*. In the case of the former, it deletes the sentinel entry from every table, and in the case of the latter, it deletes the GetNumDefinitions() method from the interface. These are no longer necessary as ArrayRef carries its own length.

The bulk of this CL is relocating the options table from one point in the file to another. This is because I removed the static OptionDefinition g_option_table from each of the classes. By raising them to the file statics instead of class statics, it should give them internal linkage, speeding up link times and possibly having positive effect on binary size. But it also means they have to appear above the option definition class instead of below, since there is no declaration.

Diff Detail

Repository
rL LLVM

Event Timeline

zturner updated this revision to Diff 72190.Sep 22 2016, 10:21 AM
zturner retitled this revision from to Convert option tables to use llvm::ArrayRefs.
zturner updated this object.
zturner added a reviewer: clayborg.
zturner added a subscriber: llvm-commits.
clayborg edited edge metadata.Sep 22 2016, 10:27 AM

My only complaint is that many changes involve moving code from line N to line M. This will make merges harder. Is there any reason you actually moved all those C option array definitions?

Previously the option tables were declared as class statics and defined below the class. And only a pointer was returned, so the compiler didn't have to know about the size of the array at compile time.

In order for the compiler to know that the OptionDefinition[] is actually an OptionDefinition[7], or whatever (which is necessary in order to set the internal size member of the ArrayRef), the definition has to come before the class.

A side benefit is that this gives the arrays internal linkage, which should improve link times. A static class member still has external linkage.

clayborg accepted this revision.Sep 22 2016, 10:45 AM
clayborg edited edge metadata.

No, just making sure there was a reason they had to move. If there is, then this is ok.

This revision is now accepted and ready to land.Sep 22 2016, 10:45 AM
This revision was automatically updated to reflect the committed changes.