Generate most of the target properties from a central specification.
AbandonedPublic

Authored by aprantl on Feb 22 2018, 3:45 PM.

Details

Summary

I recently had to add a new property and was annoyed by how much repetitive code I needed to write manually.

This patch applies a common pattern used all over LLVM to auto-generate most of the definitions and accessors for target properties from a central definition file. The change is mostly NFC and should result in a small reduction of code size.

This is by no means done, and there are many other things that could be derived in a similar way, but I would like to get the discussion started before sinking too much time into this refactoring.

It does affect the realtive ordering of the properties though.

Let me know what you think!

Diff Detail

aprantl created this revision.Feb 22 2018, 3:45 PM

What would I have to figure out if I wanted to stop where one of these properties was actually set or read? That's something I've had to do many's the time... This task is straightforward in the original, and if it's not too bad to cons up the name from this macro goo then I'm over all in favor. I'm not a big fan of macros, but what this is replacing wasn't all that pretty either.

Also (though you probably knew this) there are Properties in a lot of classes, so to do it properly you would have to have be able to move all the goo but including the individual defs files to the Properties.h for this to be viable.

What would I have to figure out if I wanted to stop where one of these properties was actually set or read? That's something I've had to do many's the time... This task is straightforward in the original, and if it's not too bad to cons up the name from this macro goo then I'm over all in favor.

You won't be able to set a breakpoint on a specific source line, but setting a breakpoint by name, such as "b TargetProperties::SetNonStopModeEnabled" has worked before and keeps working with this implementation. It will take you to Target.h:106 which is the shared source location for all boolean set functions.

I'm not a big fan of macros, but what this is replacing wasn't all that pretty either.

Yeah, this isn't great, but at least it's an established pattern that's used all over our code base.

Also (though you probably knew this) there are Properties in a lot of classes, so to do it properly you would have to have be able to move all the goo but including the individual defs files to the Properties.h for this to be viable.

Oh! I *didn't* know that. Is there a good reason to keep these properties there or would it make sense to just build a central place for all properties.

labath added a subscriber: labath.Feb 22 2018, 4:42 PM

I'm not against this in any way, but my long-term goal for this would be so that I can declare a property with something like:

Property<FileSpec> InputPath(ParentProperty, "input-path", "description-string")

where the Property class would know have an appropriate conversion operator, know how to set itself from a string, etc.

With such a setup, I'm hoping we can drive the macros and the boilerplate down to a minimum.

The question I'm asking is if I didn't know the name of the function, but I had found the found the string name of the property in the defs file, how would I figure out the name of the setter and getter? But I see that you use Get##ID so it's always going to be {Get,Set} of the first element in the HANDLE_PROPERTY. If everybody uses the rule out of Properties.h, then that will always be the case, so if we document it somewhere so people don't have to read all this goo, then I'm fine with that.

Any class can provide properties, including plugins. Since plugins may come and go (and in some future could be added dynamically) those properties don't really belong in some central repository. You are always accessing them from the containing class as well, so it makes sense to keep the actual defs file close to the class that uses them. All the properties end up being settings, and the "settings list" output reflects the containing class, so it's really easy to go from a property name in "settings" to the place where you can find it.

The one thing I worry about is people coming new to the code will see the setting target.default-arch and want to figure out how to get or set it in code. That's a bit daunting from the defs file. But I think that's just a matter of some appropriate documentation, not a serious objection.

I like Pavel's template syntax. It's not clear to me how to best implement the registering of the properties this way though. I think we want to avoid static intializers. Given how TargetProperties::TargetProperties() is implemented, I could imagine we could just make the properties members of the class, we just need some way to generate a for-each-property loop to handle the initialization. I'll think about this some more.

Thanks for your feedback!

Yes, the property would probably be a member variable of some object (maybe even of the parent property, although we would also want to have a setup where the properties can be declared outside of the parent object (for plugins, etc.)). Then something would need to instantiate that object at an appropriate time.

I have to admit I haven't looked into the feasibility of this too much, and it may turn out to be a fairly big refactor, but I think it's something worth striving for.

I like Pavel's template syntax. It's not clear to me how to best implement the registering of the properties this way though. I think we want to avoid static intializers. Given how TargetProperties::TargetProperties() is implemented, I could imagine we could just make the properties members of the class, we just need some way to generate a for-each-property loop to handle the initialization. I'll think about this some more.

We do want to avoid static initializers that happen automatically via the C++ global constructor chain, but we have the class static lldb_pricvate::*::Initialize() and that would be a perfect place to place these property definitions. I like Pavel's templated approach as well. I do second that it would be nice to be able to debug. I really dislike the .def file stuff in LLVM code. Although it is easy to maintain, you can't look at the code and figure anything out, you must preprocess the file for many things just to see what it is. I wouldn't mind it if we generated the source file from a .def file and then checked that file in so we can actually read the code...

aprantl abandoned this revision.Mon, Sep 24, 12:50 PM
aprantl added a subscriber: sgraenitz.

A