[clang-format][docs] handle explicit enum values

[kwk]

Summary

If there was this enum definition before:

struct FormatStyle {
  //...

  /// Some documentation
  enum WhateverStyle : unsigned char {
    /// Foo
    WS_Bar = 5
  };
};

We would output the following in
clang/docs/ClangFormatStyleOptions.rst
when running cd ~/llvm-project/clang/docs/tools && ./dump_format_style.py:

* ``WS_Bar = 5`` (in configuration: ``Bar = 5``)
  Foo.

With this patch, we change it to something that looks more like what we
are accustomed to:

* ``WS_Bar`` (in configuration: ``Bar``)
  Foo.

This is a theoretical change because we don't have format style enums
that currently explicitly select a value. But while I was doing some
research on how to extend the FormatStyle I noticed this behavior and
thought it would make a small change.

You can experiment with and without this change by simply running
dump_format_style.py while setting AIAS_Left to AIAS_Left = 0
in clang/include/clang/Format/Format.h. Without this change in
Format.h you shouldn't see any change being made to
clang/docs/ClangFormatStyleOptions.rst.

[MyDeveloperDay]

I'm not quite sure how I feel about adding functionality that doesn't actually do anything.

clang/docs/tools/dump_format_style.py

174–175

Feels like there is repetition here

[kwk]

In D120712#3351014, @MyDeveloperDay wrote:

I'm not quite sure how I feel about adding functionality that doesn't actually do anything.

I hear you. My reasoning was that I'm going to introduce an explicit enum value sometime soon and when I have that value and this patch in one together with the added logic it would be harder to review. Whereas with my upcoming changes I'm simply updating the clang/docs/ClangFormatStyleOptions.rst by regenerating it with this script and you can just approve it. Essentially I want to make the review process as easy as it could be.

[kwk]

clang/docs/tools/dump_format_style.py

174–175

Are you suggesting to introduce a function name_only that looks like this?

def name_only(s: str) -> str:
  return s.split("=", 1)[0].strip()

If so, should this be a member function of EnumValue or where would you see it?

I'm not fond of introducing this function TBH because the name suggest it to be very generically usable when in fact it does just a simple thing. If you pass foo and bar to it you get foo and bar but that's not a usable name of any kind, right?

[MyDeveloperDay]

My reasoning was that I'm going to introduce an explicit enum value

Any reason why?

[MyDeveloperDay]

clang/docs/tools/dump_format_style.py

174–175

yes I was suggesting adding a function, feel free to suggest a better name.

However all this said, I don't understand the need, you say your going to introduce explicit values to the enum, but why? I'm not a massive fan of that either, normally it means someone wants to abuse the enum by using the values and that IMHO can means people start doing things like taking values away from each other and/or doing a range loop from one value to another, and this becomes very unstable when enum values are added in between.

But please enlighten us as to what use the values will have I'm keen to learn.

[kwk]

In D120712#3351889, @MyDeveloperDay wrote:

My reasoning was that I'm going to introduce an explicit enum value

Any reason why?

Yes, I but where's the fun if I told you now ;) . Just kidding. I don't want to go into the details but I plan on supporting a shared value between all enumerations. That's all I can say for now before I have my PoC ready to be presented for further discussion. But even if this future patch will never land, this very patch doesn't hurt, does it? Like I said, I tried to split my work into more easily consumable patches but I guess you're not convinced.

[MyDeveloperDay]

Does it need to be so "cloak and dagger"? ;-)

We always welcome patches, but please think about logging the idea in github issues (and assigning it to yourself), and use the good will of the regular contributors to give you some feedback/advice (you never know we might know a thing or too)

I'd like to see this review in the context of what you are proposing. I know it doesn't do any actual harm on its own, but its doesn't actually do anything, as such its technical debt right? we have to maintain and support it when it goes wrong (it will unlikely go wrong, but it could I guess).

My suggestion is to hold off this patch until you have something that needs it then submit the reviews at the same time and mark with a dependency. (@curdeius, @HazardyKnusperkeks, @owenpan am I being fair?)

Please also consider being more open about what you intend to work on, you never know we could save you time and effort. Plus remember you'll want one of us to review the changes so its good to build good relationships with the key contributors and get their input.

[HazardyKnusperkeks]

In D120712#3352051, @MyDeveloperDay wrote:

Does it need to be so "cloak and dagger"? ;-)

We always welcome patches, but please think about logging the idea in github issues (and assigning it to yourself), and use the good will of the regular contributors to give you some feedback/advice (you never know we might know a thing or too)

I'd like to see this review in the context of what you are proposing. I know it doesn't do any actual harm on its own, but its doesn't actually do anything, as such its technical debt right? we have to maintain and support it when it goes wrong (it will unlikely go wrong, but it could I guess).

My suggestion is to hold off this patch until you have something that needs it then submit the reviews at the same time and mark with a dependency. (@curdeius, @HazardyKnusperkeks, @owenpan am I being fair?)

Please also consider being more open about what you intend to work on, you never know we could save you time and effort. Plus remember you'll want one of us to review the changes so its good to build good relationships with the key contributors and get their input.

No I think this is good.
I really appreciate splitting the stuff into multiple hunks, but yeah I would hold this back until we see it is needed. I'm also full of doubt why we would need the numeric values in the enums.

Please add the clang-format tag for clang-format related patches.

[HazardyKnusperkeks]

This is just a formality for the hold back. I have no opinion on the actual change. (I keep myself out of the python stuff.)

[kwk]

In D120712#3352051, @MyDeveloperDay wrote:

Does it need to be so "cloak and dagger"? ;-)

No, you're right it doesn't have to be this way.

We always welcome patches, but please think about logging the idea in github issues (and assigning it to yourself), and use the good will of the regular contributors to give you some feedback/advice (you never know we might know a thing or too)

Fair enough.

I'd like to see this review in the context of what you are proposing. I know it doesn't do any actual harm on its own, but its doesn't actually do anything, as such its technical debt right? we have to maintain and support it when it goes wrong (it will unlikely go wrong, but it could I guess).

My suggestion is to hold off this patch until you have something that needs it then submit the reviews at the same time and mark with a dependency. (@curdeius, @HazardyKnusperkeks, @owenpan am I being fair?)

That sounds like a plan.

Please also consider being more open about what you intend to work on, you never know we could save you time and effort. Plus remember you'll want one of us to review the changes so its good to build good relationships with the key contributors and get their input.

To be honest, I'm rather new to the clang-format code base and plan on adding something for which I'd like to do the work. I've took your advice and created this github issue: https://github.com/llvm/llvm-project/issues/54137 . I look forward to comments on it. I really want to establish a good relationship and I appreciate that you kindly tell me about this.

[MyDeveloperDay]

I have left, (some quite harsh opinions on this idea here, I'm sorry):
https://github.com/llvm/llvm-project/issues/54137

If you do want to contribute can I suggest you start with an item off this list, I think it will give you good insight into what is involved with making a change, you might not then think changing every option is such a brilliant idea ;-)

https://github.com/llvm/llvm-project/issues?q=is%3Aopen+label%3A%22good+first+issue%22+label%3Aclang-format

I don't want to put you off, and I don't want to discourage your contributions, Sometimes its better to just start with a issue, lets gain mutual trust and respect then the grand ideas are much easier for us to swallow, especially for those of us who have perhaps become a little more cynical. (Sorry!)

clang/docs/tools/dump_format_style.py

def __str__(self):
     return s
 
 class EnumValue(object):
   def __init__(self, name, comment, config):
     self.name = name
     self.comment = comment
     self.config = config
 
+  @property
+  def clean_name(self) -> str:
+    # In case the enum value has an explicit value (e.g. enum foo {bar = 42};)
+    # we remove everything after the equal sign and just use "bar". 
+    return self.name.split("=", 1)[0].strip()
+
+  @property
+  def clean_config(self) -> str:
+    return re.sub('.*_', '', self.config).split("=", 1)[0].strip()
+
   def __str__(self):
     return '* ``%s`` (in configuration: ``%s``)\n%s' % (
-        self.name,
-        re.sub('.*_', '', self.config),
+        self.clean_name,
+        self.clean_config,
         doxygen2rst(indent(self.comment, 2)))
 
 
 class OptionsReader:
   def __init__(self, header: TextIOWrapper):
     self.header = header
     self.in_code_block = False
     self.code_indent = 0