This is an archive of the discontinued LLVM Phabricator instance.

Format OptionEnumValueElement
ClosedPublic

Authored by JDevlieghere on Jul 30 2019, 4:47 PM.

Details

Summary

Reformat OptionEnumValueElement to make it easier to distinguish between its fields.

Diff Detail

Repository
rL LLVM

Event Timeline

JDevlieghere created this revision.Jul 30 2019, 4:47 PM
labath added a subscriber: labath.Jul 30 2019, 11:38 PM

Why do we need a separate backend for this? Couldn't this be emitted as a part of the same #include "XXXProperties.inc" which defines the property definition? The two logically belong together, and the only reason they were separate variables in the first place was the limitations of constexpr global variables...

Why do we need a separate backend for this? Couldn't this be emitted as a part of the same #include "XXXProperties.inc" which defines the property definition? The two logically belong together, and the only reason they were separate variables in the first place was the limitations of constexpr global variables...

I'm not sure what you mean by that last sentence, maybe because I don't know the history? Can you elaborate a bit on the "separate variables" part?

To answer your question: the OptionEnumValueElements are used both by properties and command options. Technically it is possible to put them in the same .td files, and emit them in the same .inc file. The actual code wouldn't change, just the way we invoke: instead of being its own backend, it would be called from both existing backends. I don't have a strong preference for either approach.

Why do we need a separate backend for this? Couldn't this be emitted as a part of the same #include "XXXProperties.inc" which defines the property definition? The two logically belong together, and the only reason they were separate variables in the first place was the limitations of constexpr global variables...

I'm not sure what you mean by that last sentence, maybe because I don't know the history? Can you elaborate a bit on the "separate variables" part?

To answer your question: the OptionEnumValueElements are used both by properties and command options. Technically it is possible to put them in the same .td files, and emit them in the same .inc file. The actual code wouldn't change, just the way we invoke: instead of being its own backend, it would be called from both existing backends. I don't have a strong preference for either approach.

Ah, I'm sorry. Somehow I didn't get what the " used by both properties and options" part means. I do now, and I agree that in this case, it does not make sense to generate these as a part of the properties definition.

However, I'm not sure it then makes sense to generate them at all. I mean, it's not like there's any redundancy (like it was for option definitions) in the OptionEnumValueElement struct, and it's pretty obvious what the individual fields mean even without naming them. And the tablegenning definitely increases the barrier for understanding the code. Is there any follow-up work or cleanups that would not be possible if this just stays a plain C array?

Why do we need a separate backend for this? Couldn't this be emitted as a part of the same #include "XXXProperties.inc" which defines the property definition? The two logically belong together, and the only reason they were separate variables in the first place was the limitations of constexpr global variables...

I'm not sure what you mean by that last sentence, maybe because I don't know the history? Can you elaborate a bit on the "separate variables" part?

To answer your question: the OptionEnumValueElements are used both by properties and command options. Technically it is possible to put them in the same .td files, and emit them in the same .inc file. The actual code wouldn't change, just the way we invoke: instead of being its own backend, it would be called from both existing backends. I don't have a strong preference for either approach.

Ah, I'm sorry. Somehow I didn't get what the " used by both properties and options" part means. I do now, and I agree that in this case, it does not make sense to generate these as a part of the properties definition.

However, I'm not sure it then makes sense to generate them at all. I mean, it's not like there's any redundancy (like it was for option definitions) in the OptionEnumValueElement struct, and it's pretty obvious what the individual fields mean even without naming them. And the tablegenning definitely increases the barrier for understanding the code. Is there any follow-up work or cleanups that would not be possible if this just stays a plain C array?

I don't have any cleanups planned for now. My motivation is purely aesthetical: I don't like the // clang-format off markers and think the C arrays look messy with the multiline oddly broken up strings. Anyway, I just mention it for context. I don't want to push this through if the consensus is that this is overkill.

I don't have any cleanups planned for now. My motivation is purely aesthetical: I don't like the // clang-format off markers and think the C arrays look messy with the multiline oddly broken up strings. Anyway, I just mention it for context. I don't want to push this through if the consensus is that this is overkill.

Yeah, the C arrays aren't the prettiest sight, but OTOH your tablegen files don't respect the column limit, and so if you view them with a line-wrapping editor (such as this phabricator page), they don't look particularly nice either.

I'm not sure the result is better, but it is possible to control the way clang-format lays out arrays like this via trailing commas. If you include a trailing comma, it will put each entry on a separate line, which may be better for those long strings (though that's highly subjective). E.g.:

static constexpr OptionEnumValueElement s_stop_show_column_values[] = {
    {
        eStopShowColumnAnsiOrCaret,
        "ansi-or-caret",
        "Highlight the stop column with ANSI terminal codes when color/ANSI "
        "mode is enabled; otherwise, fall back to using a text-only caret (^) "
        "as if \"caret-only\" mode was selected.",
    },
    {
        eStopShowColumnAnsi,
        "ansi",
        "Highlight the stop column with ANSI terminal codes when running LLDB "
        "with color/ANSI enabled.",
    },
    {
        eStopShowColumnCaret,
        "caret",
        "Highlight the stop column with a caret character (^) underneath the "
        "stop column. This method introduces a new line in source listings "
        "that display thread stop locations.",
    },
    {
        eStopShowColumnNone,
        "none",
        "Do not highlight the stop column.",
    },
};

instead of:

static constexpr OptionEnumValueElement s_stop_show_column_values[] = {
    {eStopShowColumnAnsiOrCaret, "ansi-or-caret",
     "Highlight the stop column with ANSI terminal codes when color/ANSI mode "
     "is enabled; otherwise, fall back to using a text-only caret (^) as if "
     "\"caret-only\" mode was selected."},
    {eStopShowColumnAnsi, "ansi",
     "Highlight the stop column with ANSI terminal codes when running LLDB "
     "with color/ANSI enabled."},
    {eStopShowColumnCaret, "caret",
     "Highlight the stop column with a caret character (^) underneath the stop "
     "column. This method introduces a new line in source listings that "
     "display thread stop locations."},
    {eStopShowColumnNone, "none", "Do not highlight the stop column."}};

I don't have any cleanups planned for now. My motivation is purely aesthetical: I don't like the // clang-format off markers and think the C arrays look messy with the multiline oddly broken up strings. Anyway, I just mention it for context. I don't want to push this through if the consensus is that this is overkill.

Yeah, the C arrays aren't the prettiest sight, but OTOH your tablegen files don't respect the column limit, and so if you view them with a line-wrapping editor (such as this phabricator page), they don't look particularly nice either.

True, but at least you can grep without having to guess where the ling break might have ended up :-)

I'm not sure the result is better, but it is possible to control the way clang-format lays out arrays like this via trailing commas. If you include a trailing comma, it will put each entry on a separate line, which may be better for those long strings (though that's highly subjective). E.g.:

static constexpr OptionEnumValueElement s_stop_show_column_values[] = {
    {
        eStopShowColumnAnsiOrCaret,
        "ansi-or-caret",
        "Highlight the stop column with ANSI terminal codes when color/ANSI "
        "mode is enabled; otherwise, fall back to using a text-only caret (^) "
        "as if \"caret-only\" mode was selected.",
    },
    {
        eStopShowColumnAnsi,
        "ansi",
        "Highlight the stop column with ANSI terminal codes when running LLDB "
        "with color/ANSI enabled.",
    },
    {
        eStopShowColumnCaret,
        "caret",
        "Highlight the stop column with a caret character (^) underneath the "
        "stop column. This method introduces a new line in source listings "
        "that display thread stop locations.",
    },
    {
        eStopShowColumnNone,
        "none",
        "Do not highlight the stop column.",
    },
};

This looks much, much better imho. I can live with reformatting them if we don't want to go the tablegen route.

Yeah, the C arrays aren't the prettiest sight, but OTOH your tablegen files don't respect the column limit, and so if you view them with a line-wrapping editor (such as this phabricator page), they don't look particularly nice either.

True, but at least you can grep without having to guess where the ling break might have ended up :-)

Yeah, the grepping thing isn't nice, but I think working around that by putting the strings into tablegen is cheating -- you're exploiting the fact that we don't have a code formatting tool nor a formal style guide for tablegen files. A lot of the official style guide cannot be directly applied to the tablegen files, but the column limit is one of the few things that can. :D

It looks like llvm tablegen files are not strictly adhering to that, but they are written with that limit in mind -- ascii art is 80 chars wide, and only 2.5% of lines are 81 chars or over.

I'm not sure the result is better, but it is possible to control the way clang-format lays out arrays like this via trailing commas. If you include a trailing comma, it will put each entry on a separate line, which may be better for those long strings (though that's highly subjective). E.g.:

static constexpr OptionEnumValueElement s_stop_show_column_values[] = {
    {
        eStopShowColumnAnsiOrCaret,
        "ansi-or-caret",
        "Highlight the stop column with ANSI terminal codes when color/ANSI "
        "mode is enabled; otherwise, fall back to using a text-only caret (^) "
        "as if \"caret-only\" mode was selected.",
    },
    {
        eStopShowColumnAnsi,
        "ansi",
        "Highlight the stop column with ANSI terminal codes when running LLDB "
        "with color/ANSI enabled.",
    },
    {
        eStopShowColumnCaret,
        "caret",
        "Highlight the stop column with a caret character (^) underneath the "
        "stop column. This method introduces a new line in source listings "
        "that display thread stop locations.",
    },
    {
        eStopShowColumnNone,
        "none",
        "Do not highlight the stop column.",
    },
};

This looks much, much better imho. I can live with reformatting them if we don't want to go the tablegen route.

Great, then I propose we go for that. :)

JDevlieghere retitled this revision from Tablegen option enum value elements to Format OptionEnumValueElement.
JDevlieghere edited the summary of this revision. (Show Details)
JDevlieghere added a reviewer: labath.

Reformat OptionEnumValueElement instead of using tablegen.

jingham added a subscriber: jingham.Aug 1 2019, 3:06 PM

What happens if you clang-format your reformatted version of the setter? Do we need clang-format off for them?

Otherwise this looks fine to me.

What happens if you clang-format your reformatted version of the setter? Do we need clang-format off for them?

Otherwise this looks fine to me.

This is the clang-format output!

jingham accepted this revision.Aug 1 2019, 4:59 PM

Okay, if clang-format won't undo this, then I'm fine with writing them this way.

This revision is now accepted and ready to land.Aug 1 2019, 4:59 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 1 2019, 5:18 PM