[clang-format] Option to ignore macro definitions #70338

tomekpaszek · 2023-10-26T14:24:32Z

Sometimes macro definitions contain a layout we don't want to change in any context. This PR adds a new style option IgnorePPDefinitions that prevents clang-format from touching macro definitions.

This PR addresses issue #67991

github-actions · 2023-10-27T12:10:27Z

✅ With the latest revision this PR passed the C/C++ code formatter.

clang/include/clang/Format/Format.h

clang/lib/Format/UnwrappedLineFormatter.cpp

clang/unittests/Format/FormatTest.cpp

rymiel · 2023-10-28T01:07:41Z

#67911 is a pull request, not an issue, i'm not sure what you're referring to

owenca

Please refer to the policy for adding new options.

owenca · 2023-10-28T01:15:22Z

#67911 is a pull request, not an issue, i'm not sure what you're referring to

I think it was a typo. It should be #67991.

mydeveloperday · 2023-10-30T13:47:06Z

clang/unittests/Format/FormatTest.cpp

let not leave TODO's lets just do it..

The code path for when the statement is preceded by a comment seems different. It's not 100% unclear to me where to look to support that. Any hints would be appriciated

I always comment every other check out, and run only this test case in a debugger to look where the change is needed.

Actually tests with a (line and block) comment after the #define would be interesting too, should they be wrapped? Or the space after // added?

clang/include/clang/Format/Format.h

tru · 2023-11-10T13:50:01Z

@tomekpaszek do you plan to finish this PR? we have been testing it internally on our code and would really like this to land in upstream LLVM.

tomekpaszek · 2023-11-10T16:00:54Z

Yes, I just couldn't find time for it. I'll try to look into it within the next few days. Great to hear you there is interest in using it!

clang/include/clang/Format/Format.h

clang/unittests/Format/FormatTest.cpp

clang/lib/Format/UnwrappedLineParser.cpp

HazardyKnusperkeks · 2023-11-12T19:38:51Z

clang/unittests/Format/FormatTest.cpp

I always comment every other check out, and run only this test case in a debugger to look where the change is needed.

HazardyKnusperkeks · 2023-11-12T19:40:30Z

clang/unittests/Format/FormatTest.cpp

Actually tests with a (line and block) comment after the #define would be interesting too, should they be wrapped? Or the space after // added?

clang/lib/Format/UnwrappedLineFormatter.cpp

clang/include/clang/Format/Format.h

clang/unittests/Format/FormatTest.cpp

llvmbot · 2023-11-13T09:59:13Z

@llvm/pr-subscribers-clang

Author: Tomek (tomekpaszek)

Changes

Sometimes macro definitions contain a layout we don't want to change in any context. This PR adds a new style option IgnorePPDefinitions that prevents clang-format from touching PP directives.

This PR addresses issue #67991

Full diff: https://github.com/llvm/llvm-project/pull/70338.diff

7 Files Affected:

(modified) clang/docs/ClangFormatStyleOptions.rst (+5)
(modified) clang/include/clang/Format/Format.h (+5)
(modified) clang/lib/Format/Format.cpp (+2)
(modified) clang/lib/Format/UnwrappedLineFormatter.cpp (+2)
(modified) clang/lib/Format/UnwrappedLineParser.cpp (+8)
(modified) clang/unittests/Format/ConfigParseTest.cpp (+1)
(modified) clang/unittests/Format/FormatTest.cpp (+47)

diff --git a/clang/docs/ClangFormatStyleOptions.rst b/clang/docs/ClangFormatStyleOptions.rst
index 21342e1b89ea866..80565620d8f24bf 100644
--- a/clang/docs/ClangFormatStyleOptions.rst
+++ b/clang/docs/ClangFormatStyleOptions.rst
@@ -3160,6 +3160,11 @@ the configuration (without a prefix: ``Auto``).
   For example: `KJ_IF_MAYBE
   <https://github.com/capnproto/capnproto/blob/master/kjdoc/tour.md#maybes>`_
 
+.. _IgnorePPDefinitions:
+
+**IgnorePPDefinitions** (``Boolean``) :versionbadge:`clang-format 18` :ref:`¶ <IgnorePPDefinitions>`
+  Ignore formatting in preprocessor definitions.
+
 .. _IncludeBlocks:
 
 **IncludeBlocks** (``IncludeBlocksStyle``) :versionbadge:`clang-format 6` :ref:`¶ <IncludeBlocks>`
diff --git a/clang/include/clang/Format/Format.h b/clang/include/clang/Format/Format.h
index 3e9d1915badd87f..3af7241441c8b13 100644
--- a/clang/include/clang/Format/Format.h
+++ b/clang/include/clang/Format/Format.h
@@ -2442,6 +2442,10 @@ struct FormatStyle {
   /// <https://github.com/capnproto/capnproto/blob/master/kjdoc/tour.md#maybes>`_
   /// \version 13
   std::vector<std::string> IfMacros;
+      
+  /// Ignore formatting in preprocessor definitions.
+  /// \version 18
+  bool IgnorePPDefinitions;
 
   /// Specify whether access modifiers should have their own indentation level.
   ///
@@ -4719,6 +4723,7 @@ struct FormatStyle {
                R.IncludeStyle.IncludeIsMainRegex &&
            IncludeStyle.IncludeIsMainSourceRegex ==
                R.IncludeStyle.IncludeIsMainSourceRegex &&
+           IgnorePPDefinitions == R.IgnorePPDefinitions &&
            IndentAccessModifiers == R.IndentAccessModifiers &&
            IndentCaseBlocks == R.IndentCaseBlocks &&
            IndentCaseLabels == R.IndentCaseLabels &&
diff --git a/clang/lib/Format/Format.cpp b/clang/lib/Format/Format.cpp
index edb33f4af4defef..6e5ec754dfdcdd9 100644
--- a/clang/lib/Format/Format.cpp
+++ b/clang/lib/Format/Format.cpp
@@ -1000,6 +1000,7 @@ template <> struct MappingTraits<FormatStyle> {
     IO.mapOptional("FixNamespaceComments", Style.FixNamespaceComments);
     IO.mapOptional("ForEachMacros", Style.ForEachMacros);
     IO.mapOptional("IfMacros", Style.IfMacros);
+    IO.mapOptional("IgnorePPDefinitions", Style.IgnorePPDefinitions);
     IO.mapOptional("IncludeBlocks", Style.IncludeStyle.IncludeBlocks);
     IO.mapOptional("IncludeCategories", Style.IncludeStyle.IncludeCategories);
     IO.mapOptional("IncludeIsMainRegex", Style.IncludeStyle.IncludeIsMainRegex);
@@ -1504,6 +1505,7 @@ FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
   LLVMStyle.ForEachMacros.push_back("Q_FOREACH");
   LLVMStyle.ForEachMacros.push_back("BOOST_FOREACH");
   LLVMStyle.IfMacros.push_back("KJ_IF_MAYBE");
+  LLVMStyle.IgnorePPDefinitions = false;
   LLVMStyle.IncludeStyle.IncludeCategories = {
       {"^\"(llvm|llvm-c|clang|clang-c)/", 2, 0, false},
       {"^(<|\"(gtest|gmock|isl|json)/)", 3, 0, false},
diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp b/clang/lib/Format/UnwrappedLineFormatter.cpp
index 280485d9a90d1bf..bbf6383ff7673f6 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -1355,6 +1355,8 @@ unsigned UnwrappedLineFormatter::format(
     bool FixIndentation = (FixBadIndentation || ContinueFormatting) &&
                           Indent != TheLine.First->OriginalColumn;
     bool ShouldFormat = TheLine.Affected || FixIndentation;
+    if (Style.IgnorePPDefinitions && TheLine.Type == LT_PreprocessorDirective)
+      ShouldFormat = false;
     // We cannot format this line; if the reason is that the line had a
     // parsing error, remember that.
     if (ShouldFormat && TheLine.Type == LT_Invalid && Status) {
diff --git a/clang/lib/Format/UnwrappedLineParser.cpp b/clang/lib/Format/UnwrappedLineParser.cpp
index 488d8dc07b1eae3..07cf966a08ff5e5 100644
--- a/clang/lib/Format/UnwrappedLineParser.cpp
+++ b/clang/lib/Format/UnwrappedLineParser.cpp
@@ -1134,6 +1134,14 @@ void UnwrappedLineParser::parsePPDefine() {
     return;
   }
 
+  if (Style.IgnorePPDefinitions) {
+    do {
+      nextToken();
+    } while (!eof());
+    addUnwrappedLine();
+    return;
+  }
+
   if (IncludeGuard == IG_IfNdefed &&
       IncludeGuardToken->TokenText == FormatTok->TokenText) {
     IncludeGuard = IG_Defined;
diff --git a/clang/unittests/Format/ConfigParseTest.cpp b/clang/unittests/Format/ConfigParseTest.cpp
index f90ed178d99c286..7ce9ae26b36fbb9 100644
--- a/clang/unittests/Format/ConfigParseTest.cpp
+++ b/clang/unittests/Format/ConfigParseTest.cpp
@@ -166,6 +166,7 @@ TEST(ConfigParseTest, ParsesConfigurationBools) {
   CHECK_PARSE_BOOL(DerivePointerAlignment);
   CHECK_PARSE_BOOL_FIELD(DerivePointerAlignment, "DerivePointerBinding");
   CHECK_PARSE_BOOL(DisableFormat);
+  CHECK_PARSE_BOOL(IgnorePPDefinitions);
   CHECK_PARSE_BOOL(IndentAccessModifiers);
   CHECK_PARSE_BOOL(IndentCaseLabels);
   CHECK_PARSE_BOOL(IndentCaseBlocks);
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index b2d84f2ee389551..19cf521107c604b 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -24153,6 +24153,53 @@ TEST_F(FormatTest, WhitespaceSensitiveMacros) {
   verifyNoChange("FOO(String-ized&Messy+But: :Still=Intentional);", Style);
 }
 
+TEST_F(FormatTest, IgnorePPDefinitions) {
+  FormatStyle Style = getLLVMStyle();
+  Style.IgnorePPDefinitions = true;
+
+  verifyNoChange("#define  A", Style);
+  verifyNoChange("#define A   b", Style);
+  verifyNoChange("#define A  (  args   )", Style);
+  verifyNoChange("#define A  (  args   )  =  func  (  args  )", Style);
+
+  verifyNoChange("#define A x:", Style);
+  verifyNoChange("#define A a. b", Style);
+    
+  // Surrounded with formatted code
+  verifyFormat("int a;\n"
+               "#define  A  a\n"
+               "int a;",
+               "int  a ;\n"
+               "#define  A  a\n"
+               "int  a ;",
+               Style);
+    
+  // Columns are not broken when a limit is set
+  Style.ColumnLimit = 10;
+  verifyNoChange("#define A a a a a", Style);
+  Style.ColumnLimit = 0;
+    
+  // Multiline definition
+  verifyNoChange("#define A \\\n"
+                 "Line one with spaces  .  \\\n"
+                 " Line two.",
+                   Style);
+  verifyNoChange("#define A \\\n"
+                 "a a \\\n"
+                 "a        \\\na",
+                 Style);
+  Style.AlignEscapedNewlines = FormatStyle::ENAS_Left;
+  verifyNoChange("#define A \\\n"
+                 "a a \\\n"
+                 "a        \\\na",
+                 Style);
+  Style.AlignEscapedNewlines = FormatStyle::ENAS_Right;
+  verifyNoChange("#define A \\\n"
+                 "a a \\\n"
+                 "a        \\\na", 
+                 Style);
+}
+
 TEST_F(FormatTest, VeryLongNamespaceCommentSplit) {
   // These tests are not in NamespaceEndCommentsFixerTest because that doesn't
   // test its interaction with line wrapping

clang/lib/Format/UnwrappedLineFormatter.cpp

clang/unittests/Format/FormatTest.cpp

mydeveloperday

Something here doesn't feel right

clang/unittests/Format/FormatTest.cpp

tomekpaszek

Something here doesn't feel right

Could you be more specific?

clang/lib/Format/UnwrappedLineParser.cpp

clang/lib/Format/UnwrappedLineFormatter.cpp

clang/unittests/Format/FormatTest.cpp

clang/lib/Format/UnwrappedLineFormatter.cpp

clang/unittests/Format/FormatTest.cpp

clang/lib/Format/UnwrappedLineFormatter.cpp

HazardyKnusperkeks

LGTM, but wait for either of @owenca or @mydeveloperday .

owenca

Please update ReleaseNotes.rst.

owenca · 2023-11-22T13:20:05Z

clang/include/clang/Format/Format.h

Please rename it to SkipMacroDefinition or similar.

owenca · 2023-11-22T13:24:15Z

clang/lib/Format/UnwrappedLineFormatter.cpp

Suggested change

if (Style.IgnorePPDefinitions && LineContainsPPDefinition(TheLine))

if (Style.IgnorePPDefinitions && TheLine.startsWith(tok::hash, tok::pp_define))

owenca · 2023-11-22T20:11:51Z

clang/include/clang/Format/Format.h

Suggested change

/// Ignore formatting in preprocessor definitions.

/// Do not format macro definitions.

owenca · 2023-11-22T20:17:30Z

clang/unittests/Format/FormatTest.cpp

Suggested change

FormatStyle Style = getLLVMStyle();

auto Style = getLLVMStyle();

owenca · 2023-11-23T01:47:03Z

After giving more thoughts to this, I think what we really want is SkipMacroDefinitionBody, which would format the code below:

   # define   A   a. b   //comment
   # define   A( x , y )   ( ( x ) + ( y ) )

To the following:

#define A   a. b // comment
#define A(x, y)   ( ( x ) + ( y ) )

That is, only the body (except comments) of a macro definition is not formatted.

tomekpaszek · 2023-11-27T11:41:34Z

After giving more thoughts to this, I think what we really want is SkipMacroDefinitionBody, which would format the code below:
(...)
That is, only the body (except comments) of a macro definition is not formatted.

I understand that:

we do want to align the macros (respect IndentPPDirectives)
we do want to remove extra whitespaces before, within and right after the first token (#define)
we do NOT want to touch the body
we do NOT want to break lines

owenca · 2023-11-27T12:30:02Z

After giving more thoughts to this, I think what we really want is SkipMacroDefinitionBody, which would format the code below:
(...)
That is, only the body (except comments) of a macro definition is not formatted.

I understand that:

we do want to align the macros (respect IndentPPDirectives)

we do want to remove extra whitespaces before, within and right after the first token (#define)

We want to format e.g. #define FOO and #define BAR(x, y).

we do NOT want to touch the body

we do NOT want to break lines

We don't want to change line splicing (if any) within the body.

…er tests

owenca · 2023-11-30T00:21:27Z

clang/lib/Format/UnwrappedLineParser.cpp

+  if (Style.SkipMacroDefinitionBody) {
+    do {
+      nextToken();
+    } while (!eof());
+  } else {
+    nextToken();
+  }


Instead, we can simply mark the tokens Finalized (after Line->InMacroBody is set to true below) so that the continuation indenter can be left alone.

Hi!
Maybe it's a little late, but I wanted to let you know that I've tried that but it didn't give the expected results. Didn't have time to dig into this approach.

tru · 2024-01-18T14:19:38Z

@tomekpaszek Hi Tomek, do you think you will have time to work on this soon? Otherwise, I can probably finish it off for you since we also want this feature.

tomekpaszek · 2024-01-18T14:34:26Z

@tomekpaszek Hi Tomek, do you think you will have time to work on this soon? Otherwise, I can probably finish it off for you since we also want this feature.

Hi tru,
Thanks for reaching out. Unfortunately I'm too busy to work on this at the moment. Feel free to push it forward.

tru · 2024-01-18T14:53:32Z

@owenca What's the things that still needs to be addressed for this to land?

@tomekpaszek

Closes #67991. See also: #70338 Co-authored-by: @tomekpaszek

tomekpaszek marked this pull request as ready for review October 27, 2023 12:01

llvmbot added the clang-format label Oct 27, 2023

tomekpaszek force-pushed the format-ignore-ppdirectives branch from 228301b to 1233f75 Compare October 27, 2023 12:05

HazardyKnusperkeks requested changes Oct 27, 2023

View reviewed changes

clang/include/clang/Format/Format.h Outdated Show resolved Hide resolved

clang/lib/Format/UnwrappedLineFormatter.cpp Outdated Show resolved Hide resolved

clang/unittests/Format/FormatTest.cpp Outdated Show resolved Hide resolved

HazardyKnusperkeks requested a review from owenca October 27, 2023 19:40

owenca requested review from mydeveloperday and rymiel October 28, 2023 01:05

owenca requested changes Oct 28, 2023

View reviewed changes

mydeveloperday requested changes Oct 30, 2023

View reviewed changes

tomekpaszek force-pushed the format-ignore-ppdirectives branch 2 times, most recently from 851a28a to 4ef0518 Compare November 11, 2023 20:39

HazardyKnusperkeks reviewed Nov 12, 2023

View reviewed changes

tomekpaszek force-pushed the format-ignore-ppdirectives branch from 4ef0518 to c5da2c4 Compare November 13, 2023 09:58

llvmbot added the clang Clang issues not falling into any other category label Nov 13, 2023

HazardyKnusperkeks reviewed Nov 14, 2023

View reviewed changes

clang/lib/Format/UnwrappedLineFormatter.cpp Outdated Show resolved Hide resolved

clang/unittests/Format/FormatTest.cpp Outdated Show resolved Hide resolved

mydeveloperday reviewed Nov 17, 2023

View reviewed changes

clang/unittests/Format/FormatTest.cpp Outdated Show resolved Hide resolved

tomekpaszek commented Nov 18, 2023

View reviewed changes

owenca changed the title ~~[clang-format] Option to ignore PP directives~~ [clang-format] Option to ignore macro definitions Nov 19, 2023

HazardyKnusperkeks reviewed Nov 19, 2023

View reviewed changes

clang/unittests/Format/FormatTest.cpp Outdated Show resolved Hide resolved

clang/lib/Format/UnwrappedLineFormatter.cpp Outdated Show resolved Hide resolved

clang/lib/Format/UnwrappedLineFormatter.cpp Outdated Show resolved Hide resolved

tomekpaszek requested review from HazardyKnusperkeks, mydeveloperday and owenca November 22, 2023 12:37

HazardyKnusperkeks approved these changes Nov 22, 2023

View reviewed changes

owenca reviewed Nov 22, 2023

View reviewed changes

tomekpaszek force-pushed the format-ignore-ppdirectives branch from f91b383 to 72acbae Compare November 28, 2023 15:21

tomekpaszek requested a review from owenca November 28, 2023 15:22

tomekpaszek added 15 commits November 28, 2023 16:41

Added an option to ignore macro definitions.

25ca978

Added edge cases to the test and code that fixes handling them

e19a3a7

Added more tests

4ec9846

Updated documentation files

1640ab6

Reformatted changes

4d2be79

Fixed code order

dc12a24

Added more tests to cover PP indentation and fixed readability in oth…

9debd8b

…er tests

Fixed the option affecting other PP directives

1faffeb

Added test cases for trailing comments

afdcba3

Refactored condition to ignore PP directives other than #define

3d2a56b

Fixed code style

7d73cc8

Renamed the option name to SkipMacroDefinition based on feedback

4ab55bd

Simplified condition

a0388ad

changed the logic to only ignore the body of a PP define

edc929b

renamed the setting to SkipMacroDefinitionBody

75ae623

tomekpaszek force-pushed the format-ignore-ppdirectives branch from 72acbae to 75ae623 Compare November 28, 2023 15:48

owenca reviewed Nov 30, 2023

View reviewed changes

owenca removed the clang Clang issues not falling into any other category label Nov 30, 2023

Merge branch 'main' into format-ignore-ppdirectives

9b07aca

owenca mentioned this pull request Jan 19, 2024

[clang-format] Add SkipMacroDefinitionBody option #78682

Merged

owenca added a commit that referenced this pull request Jan 20, 2024

[clang-format] Add SkipMacroDefinitionBody option (#78682)

a7d7da6

Closes #67991. See also: #70338 Co-authored-by: @tomekpaszek

owenca closed this Jan 20, 2024

	if (Style.IgnorePPDefinitions && LineContainsPPDefinition(TheLine))
	if (Style.IgnorePPDefinitions && TheLine.startsWith(tok::hash, tok::pp_define))

	/// Ignore formatting in preprocessor definitions.
	/// Do not format macro definitions.

	FormatStyle Style = getLLVMStyle();
	auto Style = getLLVMStyle();

[clang-format] Option to ignore macro definitions #70338

[clang-format] Option to ignore macro definitions #70338

Uh oh!

Conversation

tomekpaszek commented Oct 26, 2023 • edited by owenca Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rymiel commented Oct 28, 2023

Uh oh!

owenca left a comment

Choose a reason for hiding this comment

Uh oh!

owenca commented Oct 28, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tru commented Nov 10, 2023

Uh oh!

tomekpaszek commented Nov 10, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

llvmbot commented Nov 13, 2023

Uh oh!

Uh oh!

Uh oh!

mydeveloperday left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tomekpaszek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

HazardyKnusperkeks left a comment

Choose a reason for hiding this comment

Uh oh!

owenca left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

owenca commented Nov 23, 2023

Uh oh!

tomekpaszek commented Oct 26, 2023 •

edited by owenca

Loading

github-actions bot commented Oct 27, 2023 •

edited

Loading