Page MenuHomePhabricator

Move TargetLibraryInfo data from two files into one common .def file.
ClosedPublic

Authored by jvoung on Feb 23 2015, 5:25 PM.

Details

Summary

This makes it more obvious that the enum definition and the
"StandardName" array is in sync. Mechanically refactored w/ a
python script.

Diff Detail

Repository
rL LLVM

Event Timeline

jvoung updated this revision to Diff 20558.Feb 23 2015, 5:25 PM
jvoung retitled this revision from to Move TargetLibraryInfo data from two files into one common .def file..
jvoung updated this object.
jvoung edited the test plan for this revision. (Show Details)
jvoung added a subscriber: Unknown Object (MLST).
echristo added a subscriber: echristo.

Might be nicer to just have the user define whether or not they want TLI_enum or TLI_string for it?

-eric

jvoung updated this revision to Diff 20650.Feb 24 2015, 5:40 PM

Split into TLI_HANDLE_ENUM, TLI_HANDLE_STRING macros so user can opt in separately.

echristo edited edge metadata.Feb 25 2015, 11:17 AM

Hrm. I was thinking something more like:

#define TLI_HANDLE_ENUM
#include TargetLibraryInfo.def

and then inside have the actual definitions of how to handle them.

Probably want a check to make sure both aren't defined at once as well.

Thoughts?

-eric

Hmm, I don't have strong feelings either way, except that I think the enum and the string in the .def file should be clustered together so that it remains clear both are simultaneously defined whenever adding a TLI function.

I guess there is a spectrum of flexibility vs doing exactly what is needed for the current uses w/ strict error checking.

Most flexible: what I had before. Basically a for-each macro where the user can do whatever with the enum name and the string (e.g., could fill in a bunch of cases in a switch statement, not just define the enums / string table).

Middle: What I have now. Can't for-each on both the enum and the string simultaneously, but can for-each individually. This still has a bit of flexibility so that e.g., you can put the comma earlier or later

enum Func {

InvalidEnum

#define TLI_HANDLE_ENUM(enum_variant) , enum_variant
#include "llvm/Analysis/TargetLibraryInfo.def"
}

vs

enum Func {
#define TLI_HANDLE_ENUM(enum_variant) enum_variant,
#include "llvm/Analysis/TargetLibraryInfo.def"

NumLibraryFuncs

}

Strict:
#define TLI_HANDLE_ENUM
#include "llvm/Analysis/TargetLibraryInfo.def"

And then internally, "llvm/Analysis/TargetLibraryInfo.def" will check that only one of the TLI_HANDLE_* are defined at a time, and probably define some other local macros so that we still get the clustering of the enum and the string in the .def file.

As far as consistency w/ other .def files, from some sample of the existing .def files, they seem to follow the "Middle" option, though I guess many .def files are used for more complicated constructs.

It's fine any of the ways you've got it, I'd gone toward strict just to make it easier to consume and reason about for now since there's no need for the flexibility (afaict), but any of them work.

Thanks!

-eric

jvoung updated this revision to Diff 21058.Mar 2 2015, 3:58 PM
jvoung edited edge metadata.

Make the .def handle the definition details, and the .h/.cpp just do "#define TLI_DEFINE_ENUM ... #include ..."

jvoung added a comment.Mar 2 2015, 4:00 PM

Okay, strict sounds good to me. It should be pretty easy to switch to something more flexible later if needed.

echristo accepted this revision.Mar 3 2015, 11:22 AM
echristo edited edge metadata.

LGTM. Thanks for the patience!

-eric

This revision is now accepted and ready to land.Mar 3 2015, 11:22 AM
This revision was automatically updated to reflect the committed changes.