[clang-format] [PR51412] AlignConsecutiveMacros fights with Visual Studio and resource.h

[MyDeveloperDay]

Summary

https://bugs.llvm.org/show_bug.cgi?id=51412

clang-format AlignConsecutiveMacros feature causes real problems when using Win32 resource.h files via the resource editor in Visual Studio when editing RC files.

VS produces the following for the resource.h files

...
// Microsoft Visual C++ generated include file.
// Used by MyTest.rc
//
#define IDP_OLE_INIT_FAILED             100
#define IDP_FAILED_TO_CREATE            102
#define ID_STATUSBAR_SHOW               108
#define ID_STATUSBAR_TEXT               109

Visual Studio generates a resource.h with Alignment macros which start at 40 characters, but AlignConsecutiveMacros will determine the starting point but assume a minimum of 0 meaning a clang-format will result in:

#define IDP_OLE_INIT_FAILED  100
#define IDP_FAILED_TO_CREATE 102
#define ID_STATUSBAR_SHOW    108
#define ID_STATUSBAR_TEXT    109

This is would be good until you make a new rc file change which results in VS writing out the resource.h (back in its own form) - (even clang-format VS plugin to format on save doesn't resolve this. The writing of resource.h seems outside of the normal save system)

This situation is made worse, if it encounters a long symbol, in the VS case it treats this as a one off, but clang-format will assume this is the new minimum.

#define IDP_OLE_INIT_FAILED             100
#define IDP_FAILED_TO_CREATE            102
#define ID_VERYVERYVERYVERY_LONG_LONG_LONG_LONG_RESOURCE 33221
#define ID_STATUSBAR_SHOW               108
#define ID_STATUSBAR_TEXT               109

and will become via clang-format

#define IDP_OLE_INIT_FAILED                              100
#define IDP_FAILED_TO_CREATE                             102
#define ID_VERYVERYVERYVERY_LONG_LONG_LONG_LONG_RESOURCE 33221
#define ID_STATUSBAR_SHOW                                108
#define ID_STATUSBAR_TEXT                                109

This patch contains 2 new options

1. AlignConsecutiveMacrosMinWidth Sets the minimum to a fixed integer (which for VS should be 40), this means all id numbers will begin at 40
2. AlignConsecutiveMacrosIgnoreMax is an option to allow very long macro names to break the Alignment, meaning the next line will reset back to the previous configured minimum (40 in our case)

These 2 option have functionality that could be useful on their own, however the goal is to allow AlignConsecutiveMacros to be useful for Windows Win32 developers and minimize the clang-format changes that the RC editor causes.

[HazardyKnusperkeks]

Otherwise it looks good.

clang/include/clang/Format/Format.h

257

I before M, so please reorder. :)

[MyDeveloperDay]

In D112019#3073628, @HazardyKnusperkeks wrote:

Otherwise it looks good.

Thanks @HazardyKnusperkeks

[curdeius]

Please review the descriptions.
Looks good, but I'm wondering if there are better names for these options.
E.g. when AlignConsecutiveMacrosIgnoreMax is true, then AlignConsecutiveMacrosMinWidth is not minimum width, but required/fixed width except for long macro names.

clang/docs/ClangFormatStyleOptions.rst

570–573

Please add full stops at the end of sentences and fix capitals (like in With). (in Format.h of course and regenerate)
Commenting here as it's easier to see the end result.
Please check descriptions of other options too.

[MyDeveloperDay]

Address review comments and update for current trunk (up the first supported version in the documentation)

[curdeius]

LGTM.

[HazardyKnusperkeks]

Why limit to macros, could it be a member of AlignConsecutiveStyle and apply to the other stuff as well?

clang/include/clang/Format/Format.h

256

Version 15

clang/lib/Format/Format.cpp

1154

I before M

[MyDeveloperDay]

In D112019#3378024, @HazardyKnusperkeks wrote:

Why limit to macros, could it be a member of AlignConsecutiveStyle and apply to the other stuff as well?

I personally don't have a use case other than the resource.h case that I raise above, do you think this would be useful to be elsewhere in the other AlignConsecutive cases?

Of course, this code alters AlignMacroSequence() and not AlignTokens() what if we unified the options (so they were part of the struct? but kept the functionality change in separate reviews? for now the options would have no effect on other Consecutive options?

[HazardyKnusperkeks]

In D112019#3378031, @MyDeveloperDay wrote:

In D112019#3378024, @HazardyKnusperkeks wrote:

Why limit to macros, could it be a member of AlignConsecutiveStyle and apply to the other stuff as well?

I personally don't have a use case other than the resource.h case that I raise above, do you think this would be useful to be elsewhere in the other AlignConsecutive cases?

Of course, this code alters AlignMacroSequence() and not AlignTokens() what if we unified the options (so they were part of the struct? but kept the functionality change in separate reviews? for now the options would have no effect on other Consecutive options?

I don't know if there will be a use case. The more I think about it, it maybe harder to apply it to the other options.
I withdraw my comment. :)

clang/docs/ClangFormatStyleOptions.rst

.. code-block:: c++
        #define SHORT_NAME       42
        #define LONGER_NAME      0x007f
        #define EVEN_LONGER_NAME (2)
 
        #define foo(x)           (x * x)
        /* some comment */
        #define bar(y, z)        (y + z)
 
+**AlignConsecutiveMacrosIgnoreMax** (``Boolean``) :versionbadge:`clang-format 15`
+  Macros beyond ``AlignConsecutiveMacrosMinWidth`` break the
+  ConsecutiveMacroAlignment streak. When used in conjuction
+  with ``AlignConsecutiveMacrosMinWidth`` this allows longer macros to
+  not cause shorter macros to align.
+
+
+  .. code-block:: c++
+
+    #define IDC_OK          1
+    #define IDC_VERYVERYVERY_LONG 3
+    #define IDC_CANCEL      2
+
+**AlignConsecutiveMacrosMinWidth** (``Unsigned``) :versionbadge:`clang-format 14`
+  Set the Minimum macro alignment width to be used (default 0)
+  When used with ``AlignConsecutiveMacros``
+
+  Possible values:
+
+  (default 0 or not specified)
+
+  .. code-block:: c++
+
+    #define IDC_OK     1
+    #define IDC_CANCEL 2
+
+  When ``AlignConsecutiveMacrosMinWidth`` is set to 40
+
+
+  .. code-block:: c++
+
+    #define IDC_OK                          1
+    #define IDC_CANCEL                      2
+
 **AlignEscapedNewlines** (``EscapedNewlineAlignmentStyle``) :versionbadge:`clang-format 5`
   Options for aligning backslashes in escaped newlines.
 
   Possible values:
 
   * ``ENAS_DontAlign`` (in configuration: ``DontAlign``)
     Don't align escaped newlines.
 

clang/include/clang/Format/Format.h

struct FormatStyle {
   ///
   ///      #define foo(x)           (x * x)
   ///      /* some comment */
   ///      #define bar(y, z)        (y + z)
   ///    \endcode
   /// \version 9
   AlignConsecutiveStyle AlignConsecutiveMacros;
 
+  /// Macros beyond ``AlignConsecutiveMacrosMinWidth`` break the
+  /// ConsecutiveMacroAlignment streak. When used in conjuction
+  /// with ``AlignConsecutiveMacrosMinWidth`` this allows longer macros to
+  /// not cause shorter macros to align.
+  ///
+  /// \code
+  ///   #define IDC_OK          1
+  ///   #define IDC_VERYVERYVERY_LONG 3
+  ///   #define IDC_CANCEL      2
+  /// \endcode
+  /// \version 15
+  bool AlignConsecutiveMacrosIgnoreMax;
+
+  /// Set the Minimum macro alignment width to be used (default 0)
+  /// When used with ``AlignConsecutiveMacros``
+  ///
+  /// Possible values:
+  ///
+  /// (default 0 or not specified)
+  /// \code
+  ///   #define IDC_OK     1
+  ///   #define IDC_CANCEL 2
+  /// \endcode
+  ///
+  /// When ``AlignConsecutiveMacrosMinWidth`` is set to 40
+  ///
+  /// \code
+  ///   #define IDC_OK                          1
+  ///   #define IDC_CANCEL                      2
+  /// \endcode
+  /// \version 14
+  unsigned AlignConsecutiveMacrosMinWidth;
+
   /// Style of aligning consecutive assignments.
   ///
   /// ``Consecutive`` will result in formattings like:
   /// \code
   ///   int a            = 1;
   ///   int somelongname = 2;
   ///   double c         = 3;
   /// \endcode

clang/lib/Format/Format.cpp

if (!IO.outputting()) {
       IO.mapOptional("SpaceAfterControlStatementKeyword",
                      Style.SpaceBeforeParens);
     }
 
     IO.mapOptional("AccessModifierOffset", Style.AccessModifierOffset);
     IO.mapOptional("AlignAfterOpenBracket", Style.AlignAfterOpenBracket);
     IO.mapOptional("AlignArrayOfStructures", Style.AlignArrayOfStructures);
     IO.mapOptional("AlignConsecutiveMacros", Style.AlignConsecutiveMacros);
+    IO.mapOptional("AlignConsecutiveMacrosIgnoreMax",
+                   Style.AlignConsecutiveMacrosIgnoreMax);
+    IO.mapOptional("AlignConsecutiveMacrosMinWidth",
+                   Style.AlignConsecutiveMacrosMinWidth);
+
     IO.mapOptional("AlignConsecutiveAssignments",
                    Style.AlignConsecutiveAssignments);
     IO.mapOptional("AlignConsecutiveBitFields",
                    Style.AlignConsecutiveBitFields);
     IO.mapOptional("AlignConsecutiveDeclarations",
                    Style.AlignConsecutiveDeclarations);
     IO.mapOptional("AlignEscapedNewlines", Style.AlignEscapedNewlines);
     IO.mapOptional("AlignOperands", Style.AlignOperands);
FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
   LLVMStyle.AlignAfterOpenBracket = FormatStyle::BAS_Align;
   LLVMStyle.AlignArrayOfStructures = FormatStyle::AIAS_None;
   LLVMStyle.AlignOperands = FormatStyle::OAS_Align;
   LLVMStyle.AlignTrailingComments = true;
   LLVMStyle.AlignConsecutiveAssignments = FormatStyle::ACS_None;
   LLVMStyle.AlignConsecutiveBitFields = FormatStyle::ACS_None;
   LLVMStyle.AlignConsecutiveDeclarations = FormatStyle::ACS_None;
   LLVMStyle.AlignConsecutiveMacros = FormatStyle::ACS_None;
+  LLVMStyle.AlignConsecutiveMacrosMinWidth = 0;
+  LLVMStyle.AlignConsecutiveMacrosIgnoreMax = false;
   LLVMStyle.AllowAllArgumentsOnNextLine = true;
   LLVMStyle.AllowAllParametersOfDeclarationOnNextLine = true;
   LLVMStyle.AllowShortEnumsOnASingleLine = true;
   LLVMStyle.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_All;
   LLVMStyle.AllowShortBlocksOnASingleLine = FormatStyle::SBS_Never;
   LLVMStyle.AllowShortCaseLabelsOnASingleLine = false;
   LLVMStyle.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Never;
   LLVMStyle.AllowShortLambdasOnASingleLine = FormatStyle::SLS_All;

clang/lib/Format/WhitespaceManager.cpp

 
 // Aligns a sequence of matching tokens, on the MinColumn column.
 //
 // Sequences start from the first matching token to align, and end at the
 // first token of the first line that doesn't need to be aligned.
 //
 // We need to adjust the StartOfTokenColumn of each Change that is on a line
 // containing any matching token to be aligned and located after such token.
-static void AlignMacroSequence(
-    unsigned &StartOfSequence, unsigned &EndOfSequence, unsigned &MinColumn,
-    unsigned &MaxColumn, bool &FoundMatchOnLine,
-    std::function<bool(const WhitespaceManager::Change &C)> AlignMacrosMatches,
-    SmallVector<WhitespaceManager::Change, 16> &Changes) {
+static void
+AlignMacroSequence(unsigned &StartOfSequence, unsigned &EndOfSequence,
+                   unsigned &MinColumn, unsigned &MaxColumn,
+                   bool &FoundMatchOnLine,
+                   std::function<bool(const WhitespaceManager::Change &C,
+                                      const FormatStyle &Style)>
+                       AlignMacrosMatches,
+                   SmallVector<WhitespaceManager::Change, 16> &Changes,
+                   const FormatStyle &Style) {
   if (StartOfSequence > 0 && StartOfSequence < EndOfSequence) {
 
     FoundMatchOnLine = false;
     int Shift = 0;
 
     for (unsigned I = StartOfSequence; I != EndOfSequence; ++I) {
       if (Changes[I].NewlinesBefore > 0) {
         Shift = 0;
         FoundMatchOnLine = false;
       }
 
       // If this is the first matching token to be aligned, remember by how many
       // spaces it has to be shifted, so the rest of the changes on the line are
       // shifted by the same amount
-      if (!FoundMatchOnLine && AlignMacrosMatches(Changes[I])) {
+      if (!FoundMatchOnLine && AlignMacrosMatches(Changes[I], Style)) {
         FoundMatchOnLine = true;
         Shift = MinColumn - Changes[I].StartOfTokenColumn;
         Changes[I].Spaces += Shift;
       }
 
       assert(Shift >= 0);
       Changes[I].StartOfTokenColumn += Shift;
       if (I + 1 != Changes.size())
         Changes[I + 1].PreviousEndOfTokenColumn += Shift;
     }
   }
 
-  MinColumn = 0;
+  MinColumn = Style.AlignConsecutiveMacrosMinWidth;
   MaxColumn = UINT_MAX;
   StartOfSequence = 0;
   EndOfSequence = 0;
 }
 
 void WhitespaceManager::alignConsecutiveMacros() {
   if (Style.AlignConsecutiveMacros == FormatStyle::ACS_None)
     return;
 
-  auto AlignMacrosMatches = [](const Change &C) {
+  auto AlignMacrosMatches = [](const Change &C, const FormatStyle &Style) {
     const FormatToken *Current = C.Tok;
     unsigned SpacesRequiredBefore = 1;
 
     if (Current->SpacesRequiredBefore == 0 || !Current->Previous)
       return false;
 
     Current = Current->Previous;
 
     // If token is a ")", skip over the parameter list, to the
     // token that precedes the "("
     if (Current->is(tok::r_paren) && Current->MatchingParen) {
       Current = Current->MatchingParen->Previous;
       SpacesRequiredBefore = 0;
     }
 
     if (!Current || !Current->is(tok::identifier))
       return false;
 
     if (!Current->Previous || !Current->Previous->is(tok::pp_define))
       return false;
 
+    if (Style.AlignConsecutiveMacrosIgnoreMax &&
+        C.StartOfTokenColumn > Style.AlignConsecutiveMacrosMinWidth)
+      return false;
+
     // For a macro function, 0 spaces are required between the
     // identifier and the lparen that opens the parameter list.
     // For a simple macro, 1 space is required between the
     // identifier and the first token of the defined value.
     return Current->Next->SpacesRequiredBefore == SpacesRequiredBefore;
   };
 
-  unsigned MinColumn = 0;
+  unsigned MinColumn = Style.AlignConsecutiveMacrosMinWidth;
   unsigned MaxColumn = UINT_MAX;
 
   // Start and end of the token sequence we're processing.
   unsigned StartOfSequence = 0;
   unsigned EndOfSequence = 0;
 
   // Whether a matching token has been found on the current line.
   bool FoundMatchOnLine = false;
if (Changes[I].NewlinesBefore != 0) {
           !FoundMatchOnLine &&
           !(LineIsComment && ((Style.AlignConsecutiveMacros ==
                                FormatStyle::ACS_AcrossComments) ||
                               (Style.AlignConsecutiveMacros ==
                                FormatStyle::ACS_AcrossEmptyLinesAndComments)));
 
       if (EmptyLineBreak || NoMatchBreak)
         AlignMacroSequence(StartOfSequence, EndOfSequence, MinColumn, MaxColumn,
-                           FoundMatchOnLine, AlignMacrosMatches, Changes);
+                           FoundMatchOnLine, AlignMacrosMatches, Changes,
+                           Style);
 
       // A new line starts, re-initialize line status tracking bools.
       FoundMatchOnLine = false;
       LineIsComment = true;
     }
 
     if (!Changes[I].Tok->is(tok::comment))
       LineIsComment = false;
 
-    if (!AlignMacrosMatches(Changes[I]))
+    if (!AlignMacrosMatches(Changes[I], Style))
       continue;
 
     FoundMatchOnLine = true;
 
     if (StartOfSequence == 0)
       StartOfSequence = I;
 
     unsigned ChangeMinColumn = Changes[I].StartOfTokenColumn;
     int LineLengthAfter = -Changes[I].Spaces;
     for (unsigned j = I; j != E && Changes[j].NewlinesBefore == 0; ++j)
       LineLengthAfter += Changes[j].Spaces + Changes[j].TokenLength;
     unsigned ChangeMaxColumn = Style.ColumnLimit - LineLengthAfter;
 
     MinColumn = std::max(MinColumn, ChangeMinColumn);
     MaxColumn = std::min(MaxColumn, ChangeMaxColumn);
   }
 
   EndOfSequence = I;
   AlignMacroSequence(StartOfSequence, EndOfSequence, MinColumn, MaxColumn,
-                     FoundMatchOnLine, AlignMacrosMatches, Changes);
+                     FoundMatchOnLine, AlignMacrosMatches, Changes, Style);
 }
 
 void WhitespaceManager::alignConsecutiveAssignments() {
   if (Style.AlignConsecutiveAssignments == FormatStyle::ACS_None)
     return;
 
   AlignTokens(
       Style,

clang/unittests/Format/FormatTest.cpp

EXPECT_EQ("#define a    3\n"
                    " * block comment */\n"
                    "\n"
                    "\n"
                    "#define bbbb 4\n"
                    "#define ccc       (5)",
                    Style));
 }
 
+TEST_F(FormatTest, AlignConsecutiveMacrosMinWidth) {
+  FormatStyle Style = getLLVMStyle();
+  Style.AlignConsecutiveMacros = FormatStyle::ACS_Consecutive;
+  verifyFormat("#define a    3\n"
+               "#define bbbb 4\n"
+               "#define ccc  (5)",
+               Style);
+  Style.AlignConsecutiveMacrosMinWidth = 30;
+  verifyFormat("#define a                     3\n"
+               "#define bbbb                  4\n"
+               "#define ccc                   (5)",
+               Style);
+
+  verifyFormat("#define a                                     3\n"
+               "#define vvvvvvvvvverrrryyyyylonglonglongmacro 4\n"
+               "#define ccc                                   (5)",
+               Style);
+
+  Style.AlignConsecutiveMacrosIgnoreMax = true;
+  verifyFormat("#define a                     3\n"
+               "#define vvvvvvvvvverrrryyyyylonglonglongmacro 4\n"
+               "#define ccc                   (5)",
+               Style);
+}
+
 TEST_F(FormatTest, AlignConsecutiveAssignmentsAcrossEmptyLines) {
   FormatStyle Alignment = getLLVMStyle();
   Alignment.AlignConsecutiveMacros = FormatStyle::ACS_Consecutive;
   Alignment.AlignConsecutiveAssignments = FormatStyle::ACS_AcrossEmptyLines;
 
   Alignment.MaxEmptyLinesToKeep = 10;
   /* Test alignment across empty lines */
   EXPECT_EQ("int a           = 5;\n"

 #define CHECK_PARSE(TEXT, FIELD, VALUE)                                        \
   EXPECT_NE(VALUE, Style.FIELD) << "Initial value already the same!";          \
   EXPECT_EQ(0, parseConfiguration(TEXT, &Style).value());                      \
   EXPECT_EQ(VALUE, Style.FIELD) << "Unexpected value after parsing!"
 
 TEST_F(FormatTest, ParsesConfigurationBools) {
   FormatStyle Style = {};
   Style.Language = FormatStyle::LK_Cpp;
+  CHECK_PARSE_BOOL(AlignConsecutiveMacrosIgnoreMax);
   CHECK_PARSE_BOOL(AlignTrailingComments);
   CHECK_PARSE_BOOL(AllowAllArgumentsOnNextLine);
   CHECK_PARSE_BOOL(AllowAllParametersOfDeclarationOnNextLine);
   CHECK_PARSE_BOOL(AllowShortCaseLabelsOnASingleLine);
   CHECK_PARSE_BOOL(AllowShortEnumsOnASingleLine);
   CHECK_PARSE_BOOL(AllowShortLoopsOnASingleLine);
   CHECK_PARSE_BOOL(BinPackArguments);
   CHECK_PARSE_BOOL(BinPackParameters);
CHECK_PARSE("AlignConsecutiveAssignments: None", AlignConsecutiveAssignments,
               FormatStyle::ACS_None);
   CHECK_PARSE("AlignConsecutiveAssignments: Consecutive",
               AlignConsecutiveAssignments, FormatStyle::ACS_Consecutive);
   CHECK_PARSE("AlignConsecutiveAssignments: AcrossEmptyLines",
               AlignConsecutiveAssignments, FormatStyle::ACS_AcrossEmptyLines);
   CHECK_PARSE("AlignConsecutiveAssignments: AcrossEmptyLinesAndComments",
               AlignConsecutiveAssignments,
               FormatStyle::ACS_AcrossEmptyLinesAndComments);
+  CHECK_PARSE("AlignConsecutiveMacrosMinWidth: 32",
+              AlignConsecutiveMacrosMinWidth, 32u);
+
   // For backwards compability, false / true should still parse
   CHECK_PARSE("AlignConsecutiveAssignments: false", AlignConsecutiveAssignments,
               FormatStyle::ACS_None);
   CHECK_PARSE("AlignConsecutiveAssignments: true", AlignConsecutiveAssignments,
               FormatStyle::ACS_Consecutive);
 
   Style.AlignConsecutiveBitFields = FormatStyle::ACS_Consecutive;
   CHECK_PARSE("AlignConsecutiveBitFields: None", AlignConsecutiveBitFields,