[clang-format] modified goto bool to enum #65140

varshneydevansh · 2023-08-31T12:16:25Z

The basics

I branched from the main
My pull request is against main
My code follows the llvm Style Guide

The details

Resolves

Fixes proper indention for goto destination point #24492

Proposed Changes

Add a clang format style options for the goto to get the proper desired output.

Behavior Before Change

Our handling of goto labels isn't ideal, as often we are not getting the desired output and missing out the proper indentation for the got label.

Behavior After Change

The user will have three options to determine how they want their goto labels to be formatted with the 3 available options-

  enum GotoLabelIndentation {
    GLI_None,   // Do not indent goto labels.
    GLI_Indent, // Indent goto labels at the same level as the surrounding code.
    GLI_HalfIndent, // Indent goto labels at a half indent level.
  };

Test Coverage

I haven't tested this, as soon as I got confirmation that these are the required changes that will update the status of the Test Coverage.

Documentation

Documentation needs to be done for this as it's a feature request. Once this will pass all the test cases I will add the required document for this.

Additional Information

varshneydevansh · 2023-08-31T19:10:19Z

What am I Missing here?

mydeveloperday

You are missing unit tests

mydeveloperday · 2023-09-01T10:50:34Z

clang/lib/Format/Format.cpp

+    IO.enumCase(Value, "HalfIndent", FormatStyle::GLI_HalfIndent);
+  }
+};
+


You are not supporting the old true/false case this would cause everyone else to break

I'd expect to see something like this

// For backward compatibility. IO.enumCase(Value, "true", FormatStyle::GLI_Indent); IO.enumCase(Value, "false", FormatStyle::GLI_None);

working on it

Ok this should have been in addition to what you had before not instead of

clang/include/clang/Format/Format.h

mydeveloperday · 2023-09-01T10:53:03Z

clang/include/clang/Format/Format.h

@@ -2432,9 +2431,25 @@ struct FormatStyle {
  ///    label2:                                label2:
  ///      return 1;                              return 1;
  ///    }                                      }
+  ///
+  ///    GLI_HalfIndent:


where in hte code is GLI_HalfIndent handled differently from GLI_Indent sorry I'm confused

#24492 (comment)

#24492 (comment)

I added this based on the above two

clang/include/clang/Format/Format.h

mydeveloperday · 2023-09-01T10:54:57Z

Documentation needs to be done for this as it's a feature request. Once this will pass all the test cases I will add the required document for this.

This is not the way, create your feature, test it, and document it, use it yourself to ensure it works, find corner cases anf fix them, then send us the PR when it ready.

HazardyKnusperkeks · 2023-09-01T11:06:20Z

clang/include/clang/Format/Format.h

+  enum GotoLabelIndentation {
+    GLI_None,   // Do not indent goto labels.
+    GLI_Indent, // Indent goto labels at the same level as the surrounding code.
+    GLI_HalfIndent, // Indent goto labels at a half indent level.


You need more examples (with more indentation) and/or more description what you intend to do. Do you round up or down for odd indentation levels? Do you really want that, or do you only want one level less?

namespace NS { class C { void foo() { while (x) { if (y) { label: //where would this be? } } } } }

I am testing the changes

3047 | Style.IndentGotoLabels = false; in llvm-project/clang/unittests/Format/FormatTest.cpp

I missed this and it's shameful correcting it

HazardyKnusperkeks

Do you actually want to use that feature, or do you only want to work on something?

Please take a greater look on how enums are documented, parsed, and tested. A fairly recent example which transitioned from bool would be: BreakBeforeInlineASMColon

HazardyKnusperkeks · 2023-09-04T19:16:01Z

clang/unittests/Format/ConfigParseTest.cpp

@@ -161,7 +171,8 @@ TEST(ConfigParseTest, ParsesConfigurationBools) {
  CHECK_PARSE_BOOL(IndentAccessModifiers);
  CHECK_PARSE_BOOL(IndentCaseLabels);
  CHECK_PARSE_BOOL(IndentCaseBlocks);
-  CHECK_PARSE_BOOL(IndentGotoLabels);
+  CHECK_PARSE_ENUM(IndentGotoLabels,


We have dedicated sections on how to check enum parsing.

3047 | Style.IndentGotoLabels = false; in llvm-project/clang/unittests/Format/FormatTest.cpp
I missed this and it's shameful and I corrected it but now looking carefully at my local end.

I want you to also test the actual usage of your option

HazardyKnusperkeks · 2023-09-04T19:17:38Z

clang/include/clang/Format/Format.h

+  ///     void foo() {                           void foo() {
+  ///       while (x) {                            while (x) {
+  ///         if (y) {                               if (y) {
+  ///         label:                               label:


This does not display what None should do. You would break thousands of users.

looking into this

HazardyKnusperkeks · 2023-09-04T19:19:19Z

clang/docs/ClangFormatStyleOptions.rst

  Indent goto labels.

-  When ``false``, goto labels are flushed left.
+  When ``GLI_None``, goto labels are flushed left.


Do you edit this by hand?
ClangFormatStyleOptions.rst is generated by clang/docs/tools/dump_format_style.py.

So do all your edits in Format.h, and then run the script.

1> yes. I did not know that.
2> okay. Got it.

I will do it correctly in the morning.

varshneydevansh · 2023-09-04T19:33:49Z

Do you actually want to use that feature, or do you only want to work on something?

As it's something of my past pending task, I'm looking to learn and contribute in any way I can. I left without even trying to contribute to open source codes. =)

mydeveloperday

I’m not sure I quite understand what you are trying to do here now

mydeveloperday · 2023-09-04T22:31:17Z

clang/include/clang/Format/Format.h

+
+  enum GotoLabelIndentation {
+    GLI_None,   // Do not indent goto labels.
+    GLI_Indent, // Indent goto labels at the same level as the surrounding code.


Now I’m confused where did the half indent go?

mydeveloperday · 2023-09-04T22:31:28Z

clang/lib/Format/Format.cpp

+    IO.enumCase(Value, "HalfIndent", FormatStyle::GLI_HalfIndent);
+  }
+};
+


Ok this should have been in addition to what you had before not instead of

mydeveloperday · 2023-09-04T22:31:33Z

clang/unittests/Format/ConfigParseTest.cpp

@@ -161,7 +171,8 @@ TEST(ConfigParseTest, ParsesConfigurationBools) {
  CHECK_PARSE_BOOL(IndentAccessModifiers);
  CHECK_PARSE_BOOL(IndentCaseLabels);
  CHECK_PARSE_BOOL(IndentCaseBlocks);
-  CHECK_PARSE_BOOL(IndentGotoLabels);
+  CHECK_PARSE_ENUM(IndentGotoLabels,


I want you to also test the actual usage of your option

[clang] modified goto bool to enum

be006b4

tbaederr changed the title ~~[clang] modified goto bool to enum~~ [clang-format] modified goto bool to enum Sep 1, 2023

PiotrZSL added the clang-format label Sep 1, 2023

mydeveloperday requested changes Sep 1, 2023

View reviewed changes

mydeveloperday requested review from owenca, rymiel and HazardyKnusperkeks September 1, 2023 10:57

mydeveloperday added the enhancement Improving things as opposed to bug fixing, e.g. new or missing feature label Sep 1, 2023

HazardyKnusperkeks reviewed Sep 1, 2023

View reviewed changes

corrected based on feedback ran git clang-format

22acfbe

varshneydevansh requested review from a team as code owners September 4, 2023 16:27

removed trailing spaces for ci check-format

6ca74b4

HazardyKnusperkeks requested changes Sep 4, 2023

View reviewed changes

mydeveloperday requested changes Sep 4, 2023

View reviewed changes

owenca removed the enhancement Improving things as opposed to bug fixing, e.g. new or missing feature label May 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clang-format] modified goto bool to enum #65140

[clang-format] modified goto bool to enum #65140

varshneydevansh commented Aug 31, 2023

varshneydevansh commented Aug 31, 2023

mydeveloperday left a comment

mydeveloperday Sep 1, 2023 •

edited

varshneydevansh Sep 1, 2023

mydeveloperday Sep 4, 2023

mydeveloperday Sep 1, 2023

varshneydevansh Sep 1, 2023

mydeveloperday commented Sep 1, 2023 •

edited

HazardyKnusperkeks Sep 1, 2023

varshneydevansh Sep 1, 2023

varshneydevansh Sep 4, 2023

varshneydevansh Sep 4, 2023

HazardyKnusperkeks left a comment

HazardyKnusperkeks Sep 4, 2023

varshneydevansh Sep 4, 2023

mydeveloperday Sep 4, 2023

HazardyKnusperkeks Sep 4, 2023

varshneydevansh Sep 4, 2023

HazardyKnusperkeks Sep 4, 2023

varshneydevansh Sep 4, 2023

varshneydevansh Sep 4, 2023

varshneydevansh commented Sep 4, 2023

mydeveloperday left a comment

mydeveloperday Sep 4, 2023

mydeveloperday Sep 4, 2023

mydeveloperday Sep 4, 2023

[clang-format] modified goto bool to enum #65140

Are you sure you want to change the base?

[clang-format] modified goto bool to enum #65140

Conversation

varshneydevansh commented Aug 31, 2023

The basics

The details

Resolves

Proposed Changes

Behavior Before Change

Behavior After Change

Test Coverage

Documentation

Additional Information

varshneydevansh commented Aug 31, 2023

mydeveloperday left a comment

Choose a reason for hiding this comment

mydeveloperday Sep 1, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mydeveloperday commented Sep 1, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HazardyKnusperkeks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varshneydevansh commented Sep 4, 2023

mydeveloperday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mydeveloperday Sep 1, 2023 •

edited

mydeveloperday commented Sep 1, 2023 •

edited