[Lldb-commits] [PATCH] D65185: Let tablegen generate property definitions

Thu Jul 25 01:13:55 PDT 2019

labath added a comment.

In D65185#1599423 <https://reviews.llvm.org/D65185#1599423>, @JDevlieghere wrote:

> In D65185#1599262 <https://reviews.llvm.org/D65185#1599262>, @labath wrote:
>
> > .def files can omit fields too: `#define BOOL_PROPERTY(name, global, default, desc) PROPERTY(name, OptionValue::eTypeBoolean, global, default, nullptr, {}, desc)`. Some sanity checking sounds like it could be useful, but I'm not exactly sure what kind of checks you have in mind. Being able to change the representation is nice, but I expect most of those changes would also be achievable with the def files. More radical changes (like the variable thing I mentioned) would probably require changes regardless of how the properties are generated...
>
>
> I agree with you and I'm not opposed to def-files at all. I think tablegen and `.def` files have different trade-offs and while the latter could probably work for properties, I have the feeling that tablegen is a better fit. The things are mentioned before are just a few things that came to mind.
>
> To give an example of sanity checking: this isn't in the patch (yet) but with tablegen we can ensure that every option has either a default unsigned or string value. In the table you can't differentiate between a default `0` and an explicit default value of `0`.

If you used something like the BOOL_PROPERTY macro from my previous comment, then there would be no way to explicitly specify a default string value for a boolean property (and similarly for other property types too).

Overall, I'm not strongly opposed to this, but I am not convinced that the added complexity of tablegen ("#including a .def file" vs. "running a functional program which generates a def file, which is then #included") is worth the benefits. I suggest getting someone else to review this to break the stalemate. :)

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65185/new/

https://reviews.llvm.org/D65185