This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Take out common code for parsing blocks NFC
ClosedPublic

Authored by sstwcw on Mar 15 2022, 4:31 PM.

Diff Detail

Event Timeline

sstwcw created this revision.Mar 15 2022, 4:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 15 2022, 4:31 PM
sstwcw requested review of this revision.Mar 15 2022, 4:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 15 2022, 4:31 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
HazardyKnusperkeks edited reviewers, added: MyDeveloperDay, owenpan, curdeius, HazardyKnusperkeks; removed: Restricted Project.Mar 16 2022, 1:53 PM

Looks basically okay.

clang/lib/Format/UnwrappedLineParser.cpp
2841

This is completely missing. Didn't it affect anything?

clang/lib/Format/UnwrappedLineParser.h
128

I'm no fan of default arguments.
But we have them all around...

Can you add a summary as to why you are making this change? the code doesn't look the same before/after so whats changing and why? is there no unit tests that perhaps need to be added?

sstwcw marked an inline comment as done.Mar 16 2022, 4:32 PM
sstwcw added inline comments.
clang/lib/Format/UnwrappedLineParser.cpp
2841

This chunk of code is already in parseUnbracedBody.

owenpan requested changes to this revision.Mar 16 2022, 10:37 PM

Because this patch would impact inserting/removing braces, we must test it against a large codebase. Before I landed RemoveBracesLLVM and InsertBraces, I had tested them with make check-clang and compared the result with that of running the same test without the options. I'm not sure if this patch would be worth the effort as the new function only refactored code at two places.

clang/lib/Format/UnwrappedLineParser.cpp
2716–2717

The suggested names are merely my preference.

2718–2723
2725
2736
2757
2764–2765
2854

We can't refactor here as parseSwitch() doesn't call parseUnbracedBody(), which handles option InsertBraces.

clang/lib/Format/UnwrappedLineParser.h
128

There is no need to have a default for the first parameter now. See line 2839 below.

This revision now requires changes to proceed.Mar 16 2022, 10:37 PM
sstwcw marked an inline comment as done.Mar 17 2022, 5:49 AM

This patch is only intended to reduce the number of times the functionality gets implemented separately. Any change in behavior would be unintended. And we also use the parseIndentedBlock in Verilog stuff, so it's not just two places. I will wait for the check-clang result to see what to do.

Just an observation, I have this in my Day Job all the time..

  1. Developer A develops a piece of code
  2. Over the years the developers in our group learn that code and become familiar with it
  3. Developer B arrives in our group, telling us we are doing it all wrong we don't understand, but they are going to refactor it for us and correct the error of our ways.
  4. Now now one in our group understands the code, except Developer B
  5. Developer B gets annoyed at our lack of understanding of his/her more masterful refactoring skills and disappears...
  6. No one understands the code and everyone has to relearn the code. (Developer B's name is now mud!)

Refactoring is an important step, but in my view it shouldn't be the first contribution someone makes. I understand it you think its better, it possibly is.. but those of us who have been in here for years now no longer recognize the code and have to read it with a fine tooth combe again.

I personally don't see the code as being functional equal before or after, and I've kicked myself for not being more thorough in review when someone does this..that's probably my lack of understanding.

If you are fixing a bug, then ok, lets look at it, but just because you don't like the way it looks or you think there is repetition. If your an experienced contributor then maybe you'd have our trust...but otherwise we need to express caution here in my view. Just incase you chose to up and disappear.

Refactoring yes, but if its "Drive by" Refactoring, I'm not so keen.

For the new stuff I have the option of still adding the function parseIndentedBlock but only using it in new code. Please be more blunt about whether I should close this revision and do it that way. I guess I might have misunderstood you before from how you reacted when I closed the large patch.

For the new stuff I have the option of still adding the function parseIndentedBlock but only using it in new code. Please be more blunt about whether I should close this revision and do it that way. I guess I might have misunderstood you before from how you reacted when I closed the large patch.

Ok being blunt.. the things you are asking to do, look noble, I just don't always understand the motivation? you are seemingly not fixing anything, you are not adding new tests, its just moving things around, but I don't know your commitment yet? are you hanging around? are you going to fix bugs?

I feel like the guidance for new developers is do a "good first issue", the reviewers have triaged the clang-format issues to try and identify what makes a good first issue for a new contributor

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

Ultimately if you don't have commit access your going to ask one of us to land this for you, as such we might need persuading, Certainly for me to do that for you I need to see a benefit otherwise its just change in my view.

This patch is only intended to reduce the number of times the functionality gets implemented separately. Any change in behavior would be unintended. And we also use the parseIndentedBlock in Verilog stuff, so it's not just two places. I will wait for the check-clang result to see what to do.

In that case, I can accept this patch if the review comments are addressed. We can add inserting braces to switch in a separate patch. Not sure what name we should use for the function though as it would not be parsing the body of loop statements anymore.

sstwcw updated this revision to Diff 418061.Mar 24 2022, 3:45 PM
sstwcw marked 9 inline comments as done.Mar 24 2022, 4:35 PM

I tried formatting the files in clang-formatted-files.txt. Besides the files in the list that get changed when formatted with the program built from main, none gets changed when I format them with the program built from this patch, whether or not parseSwitch is modified.

owenpan added inline comments.Mar 24 2022, 7:32 PM
clang/lib/Format/UnwrappedLineParser.cpp
2721–2722

You swapped these two lines. We want to remember the l_brace before calling other functions in case they update FormatTok.

2757–2758

Or just the following as they are not default arguments now:

parseLoopBody(Style.RemoveBracesLLVM, true);
2764

Or just the following:

parseLoopBody(false, Style.BraceWrapping.BeforeWhile);

I tried formatting the files in clang-formatted-files.txt. Besides the files in the list that get changed when formatted with the program built from main, none gets changed when I format them with the program built from this patch, whether or not parseSwitch is modified.

To test a token-changing patch (e.g. inserting/removing braces) on a large codebase like clang, I would go through the following steps:

  1. Build a debug version of clang-format from scratch without applying the patch.
  2. Run the built clang-format on every .(c|cpp|h) file under clang/.
  3. Build clang.
  4. Run check-clang and save the output in a log file.
  5. Run git reset --hard and apply the patch.
  6. Build clang-format again.
  7. Add InsertBraces: true to clang/.clang-format.
  8. Repeat steps 2-4 above.
  9. Compare the new log file with the pre-patch one. They should have exactly the same Unresolved/Failed tests. They should also have the same numbers of Skipped/Unsupported/Passed/Expectedly Failed tests.

I tried formatting the files in clang-formatted-files.txt. Besides the files in the list that get changed when formatted with the program built from main, none gets changed when I format them with the program built from this patch, whether or not parseSwitch is modified.

To test a token-changing patch (e.g. inserting/removing braces) on a large codebase like clang, I would go through the following steps:

  1. Build a debug version of clang-format from scratch without applying the patch.
  2. Run the built clang-format on every .(c|cpp|h) file under clang/.
  3. Build clang.
  4. Run check-clang and save the output in a log file.
  5. Run git reset --hard and apply the patch.
  6. Build clang-format again.

And then run git reset --hard again before step 7 below.

  1. Add InsertBraces: true to clang/.clang-format.
  2. Repeat steps 2-4 above.
  3. Compare the new log file with the pre-patch one. They should have exactly the same Unresolved/Failed tests. They should also have the same numbers of Skipped/Unsupported/Passed/Expectedly Failed tests.
sstwcw updated this revision to Diff 419009.Mar 29 2022, 4:25 PM
sstwcw retitled this revision from [clang-format] Take out common code for parsing blocks to [clang-format] Take out common code for parsing blocks NFC.

I ran check-clang after formatting the entire code base with the new version. It turned out it did break some tests. It seems to be because it messed up these comments.

diff --git a/clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.fallthrough/p1.cpp b/clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.fallthrough/p1.cpp
index 7a8d756b09fd..53b2ddd12885 100644
--- a/clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.fallthrough/p1.cpp
+++ b/clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.fallthrough/p1.cpp
@@ -33,8 +33,8 @@ void f(int n) {
   case 6:
     do {
       [[fallthrough]];
-    } while (
-        false); // expected-error {{does not directly precede switch label}}
+    } while (false); // expected-error {{does not directly precede switch
+                     // label}}
   case 7:
     switch (n) {
     case 0:

Now I have undone the parts that changed this. Formatting with the new version yields the same result as formatting with the program built from the main branch, whether with or without the InsertBraces option, except for this one line:

diff --git a/clang/test/SemaCXX/constant-expression-cxx11.cpp b/clang/test/SemaCXX/constant-expression-cxx11.cpp
index 4c6b6da9e415..4d063378f37c 100644
--- a/clang/test/SemaCXX/constant-expression-cxx11.cpp
+++ b/clang/test/SemaCXX/constant-expression-cxx11.cpp
@@ -1115,7 +1115,7 @@ static_assert(check(S(5), 11), "");
 namespace PR14171 {
 
 struct X {
-  constexpr(operator int)() const { return 0; }
+  constexpr (operator int)() const { return 0; }
 };
 static_assert(X() == 0, "");

I ran check-clang after formatting the entire code base with the new version. It turned out it did break some tests. It seems to be because it messed up these comments.

diff --git a/clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.fallthrough/p1.cpp b/clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.fallthrough/p1.cpp
index 7a8d756b09fd..53b2ddd12885 100644
--- a/clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.fallthrough/p1.cpp
+++ b/clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.fallthrough/p1.cpp
@@ -33,8 +33,8 @@ void f(int n) {
   case 6:
     do {
       [[fallthrough]];
-    } while (
-        false); // expected-error {{does not directly precede switch label}}
+    } while (false); // expected-error {{does not directly precede switch
+                     // label}}
   case 7:
     switch (n) {
     case 0:

Now I have undone the parts that changed this. Formatting with the new version yields the same result as formatting with the program built from the main branch, whether with or without the InsertBraces option, except for this one line:

diff --git a/clang/test/SemaCXX/constant-expression-cxx11.cpp b/clang/test/SemaCXX/constant-expression-cxx11.cpp
index 4c6b6da9e415..4d063378f37c 100644
--- a/clang/test/SemaCXX/constant-expression-cxx11.cpp
+++ b/clang/test/SemaCXX/constant-expression-cxx11.cpp
@@ -1115,7 +1115,7 @@ static_assert(check(S(5), 11), "");
 namespace PR14171 {
 
 struct X {
-  constexpr(operator int)() const { return 0; }
+  constexpr (operator int)() const { return 0; }
 };
 static_assert(X() == 0, "");

Thanks for running check-clang. Can you address the review comments so that we can accept this patch? After you land it, I will add handling switch to InsertBraces and run check-clang again.

sstwcw updated this revision to Diff 420652.Apr 5 2022, 3:59 PM
sstwcw updated this revision to Diff 420653.Apr 5 2022, 4:03 PM
sstwcw marked 3 inline comments as done.Apr 5 2022, 4:05 PM
owenpan accepted this revision.Apr 5 2022, 5:02 PM

LGTM

This revision is now accepted and ready to land.Apr 5 2022, 5:02 PM

Would you have a look at the parent revision namely D121756?

This revision was automatically updated to reflect the committed changes.