[DRAFT][clang-format][lld] Run clang-format on lld #80461

pogo59 · 2024-02-02T16:50:52Z

Using the same critera as llvm/utils/git/code-format-helper.py, clang-format source files in the lld tree.

This is an example for https://discourse.llvm.org/t/rfc-clang-format-all-the-things/76614 and shows formatting all files, not just the small/medium files.

Instructions and a script to help downstream repos resolve conflicts will be committed separately.

Using the same critera as llvm/utils/git/code-format-helper.py, clang-format source files in the lld tree. Instructions and a script to help downstream repos resolve conflicts will be committed separately.

preames

I skimmed the first few changes here, and I see a couple of examples where auto-format is producing worse result. To me, this definitely calls for the need for human judgement on these changes. I would not approve this review as it stands.

preames · 2024-02-02T17:02:37Z

lld/COFF/Chunks.cpp

  case IMAGE_REL_AMD64_ADDR64:
    add64(off, s + imageBase);
    break;
-  case IMAGE_REL_AMD64_ADDR32NB: add32(off, s); break;


This seems to be slightly less readable, but not terrible.

This is a situation where I think having it look slightly worse is worth the general consistency autoformatting brings... But for the record you could also go for // clang-format: off:

// clang-format off case IMAGE_REL_AMD64_ADDR32NB: add32(off, s); break; case IMAGE_REL_AMD64_REL32: add32(off, s - p - 4); break; case IMAGE_REL_AMD64_REL32_1: add32(off, s - p - 5); break; case IMAGE_REL_AMD64_REL32_2: add32(off, s - p - 6); break; case IMAGE_REL_AMD64_REL32_3: add32(off, s - p - 7); break; case IMAGE_REL_AMD64_REL32_4: add32(off, s - p - 8); break; case IMAGE_REL_AMD64_REL32_5: add32(off, s - p - 9); break; // clang-format on

preames · 2024-02-02T17:03:37Z

lld/COFF/Chunks.cpp

-size_t MergeChunk::getSize() const {
-  return builder.getSize();
-}
+size_t MergeChunk::getSize() const { return builder.getSize(); }


This seems like a poor formatting choice.

One-liner get/set functions are extremely common. More often in .h files than .cpp files though. It does look odd in context. I'd be inclined to move these to the .h file actually, and keep them as single-line functions.

I agree that this is worse.
void setIsTagged and static unsigned getVerDefNum in lld/ELF/Symbols.h are similar.

There appears to be a AllowShortFunctionsOnASingleLine setting for clang-format that we can try setting: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#allowshortfunctionsonasingleline

preames · 2024-02-02T17:04:17Z

lld/COFF/Config.h

+using llvm::StringRef;
 using llvm::COFF::IMAGE_FILE_MACHINE_UNKNOWN;
 using llvm::COFF::WindowsSubsystem;
-using llvm::StringRef;


Wait, this is only clang-format right? This doesn't look like a formatting change at all.

This appears to be: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#sortusingdeclarations

and I would say sorting things where semantics don't change alphabetically (or rather by "ASCII") does more good than harm...

Looking at the doc, it appears LLVM style has been set to LexicographicNumeric, and I agree Lexicographic would be better.

Didn't mean to propose a change to "ASCII", mainly wanted to illustrate what happened. It's probably best to avoid option changes mostly.

preames · 2024-02-02T17:04:48Z

lld/COFF/Config.h

-  CV    = 0x1,  /// CodeView
-  PData = 0x2,  /// Procedure Data
-  Fixup = 0x4,  /// Relocation Table
+  None = 0x0,


This looks harmful to readability - only minorly so, admittedly.

preames · 2024-02-02T17:06:42Z

lld/COFF/DLL.cpp

+    0x51, // push  ecx
+    0x52, // push  edx
+    0x50, // push  eax
+    0x68, 0,    0,


This markedly damages readability.

should use // clang-format off here (or find other ways to express things that aren't mangled by clang-format).

static const uint8_t tailMergeX86[] = { // clang-format off 0x51, // push ecx 0x52, // push edx 0x50, // push eax 0x68, 0, 0, 0, 0, // push offset ___DELAY_IMPORT_DESCRIPTOR_<DLLNAME>_dll 0xE8, 0, 0, 0, 0, // call ___delayLoadHelper2@8 0x5A, // pop edx 0x59, // pop ecx 0xFF, 0xE0, // jmp eax // clang-format on };

or another (admittedly not too great attempt) without disabling clang format:

#define INST(x) x static const uint8_t tailMergeSequence[] = { INST(0x51), // push ecx INST(0x52), // push edx INST(0x50), // push eax INST(0x68, 0, 0, 0, 0), // push offset ___DELAY_IMPORT_DESCRIPTOR_<DLLNAME>_dll INST(0xE8, 0, 0, 0, 0), // call ___delayLoadHelper2@8 INST(0x5A), // pop edx INST(0x59), // pop ecx INST(0xFF, 0xE0), // jmp eax }

MaskRay · 2024-02-02T18:48:42Z

lld/ELF/AArch64ErrataFix.cpp

      return a->value < b->value;
    });
-    mapSyms.erase(
-        std::unique(mapSyms.begin(), mapSyms.end(),


I believe this was output from a previous version of clang-format. Personally I'd avoid such a change, but if a special rule makes things harder, I think it would be fine to format this.

MatzeB

Thanks for submitting this so we can have a constructive discussion on the effects of auto-formatting!

Generally: We should take some time and tweak clang-format settings before mass-formatting. We should also get used to more // clang-format off usage when auto-formatting. In my experience working 3 years in am auto-formatted project, you usually you have to get used to not always achieving the same level of "beauty" of human written code. But in my opinion the overal improvements is worth it for big projects with a lot of developers...

MatzeB · 2024-02-02T19:01:45Z

lld/COFF/Chunks.cpp

  case IMAGE_REL_AMD64_ADDR64:
    add64(off, s + imageBase);
    break;
-  case IMAGE_REL_AMD64_ADDR32NB: add32(off, s); break;


This is a situation where I think having it look slightly worse is worth the general consistency autoformatting brings... But for the record you could also go for // clang-format: off:

// clang-format off case IMAGE_REL_AMD64_ADDR32NB: add32(off, s); break; case IMAGE_REL_AMD64_REL32: add32(off, s - p - 4); break; case IMAGE_REL_AMD64_REL32_1: add32(off, s - p - 5); break; case IMAGE_REL_AMD64_REL32_2: add32(off, s - p - 6); break; case IMAGE_REL_AMD64_REL32_3: add32(off, s - p - 7); break; case IMAGE_REL_AMD64_REL32_4: add32(off, s - p - 8); break; case IMAGE_REL_AMD64_REL32_5: add32(off, s - p - 9); break; // clang-format on

MatzeB · 2024-02-02T19:03:27Z

lld/COFF/Chunks.cpp

-size_t MergeChunk::getSize() const {
-  return builder.getSize();
-}
+size_t MergeChunk::getSize() const { return builder.getSize(); }


There appears to be a AllowShortFunctionsOnASingleLine setting for clang-format that we can try setting: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#allowshortfunctionsonasingleline

MatzeB · 2024-02-02T19:05:16Z

lld/COFF/Config.h

+using llvm::StringRef;
 using llvm::COFF::IMAGE_FILE_MACHINE_UNKNOWN;
 using llvm::COFF::WindowsSubsystem;
-using llvm::StringRef;


This appears to be: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#sortusingdeclarations

MatzeB · 2024-02-02T19:10:32Z

lld/COFF/DLL.cpp

+    0x51, // push  ecx
+    0x52, // push  edx
+    0x50, // push  eax
+    0x68, 0,    0,


should use // clang-format off here (or find other ways to express things that aren't mangled by clang-format).

static const uint8_t tailMergeX86[] = { // clang-format off 0x51, // push ecx 0x52, // push edx 0x50, // push eax 0x68, 0, 0, 0, 0, // push offset ___DELAY_IMPORT_DESCRIPTOR_<DLLNAME>_dll 0xE8, 0, 0, 0, 0, // call ___delayLoadHelper2@8 0x5A, // pop edx 0x59, // pop ecx 0xFF, 0xE0, // jmp eax // clang-format on };

or another (admittedly not too great attempt) without disabling clang format:

#define INST(x) x static const uint8_t tailMergeSequence[] = { INST(0x51), // push ecx INST(0x52), // push edx INST(0x50), // push eax INST(0x68, 0, 0, 0, 0), // push offset ___DELAY_IMPORT_DESCRIPTOR_<DLLNAME>_dll INST(0xE8, 0, 0, 0, 0), // call ___delayLoadHelper2@8 INST(0x5A), // pop edx INST(0x59), // pop ecx INST(0xFF, 0xE0), // jmp eax }

MatzeB · 2024-02-02T19:21:01Z

lld/ELF/Arch/PPC64.cpp

-  LWA = 58,
-  STD = 62
-};
+enum DSFormOpcd { LD = 58, LWA = 58, STD = 62 };


Could add a comma here for formatting purposes:

Suggested change

enum DSFormOpcd { LD = 58, LWA = 58, STD = 62 };

enum DSFormOpcd {

LD = 58,

LWA = 58,

STD = 62,

};

MatzeB · 2024-02-02T19:26:19Z

lld/ELF/Thunks.h


-#include "llvm/ADT/SmallVector.h"
 #include "Relocations.h"
+#include "llvm/ADT/SmallVector.h"


Note include sorting here! This can technically change semantics for poorly written include files. I personally like the setting (and also the incentive to not have poorly written include files)... If you want to enforce some order you can form groups of include statements separated by empty line and each group gets sorted separately.

LLVM style has required self-contained headers for a lot longer than we've had clang-format.

lld/ELF/ScriptParser.cpp

[DRAFT][clang-format][lld] Run clang-format on lld

6519197

Using the same critera as llvm/utils/git/code-format-helper.py, clang-format source files in the lld tree. Instructions and a script to help downstream repos resolve conflicts will be committed separately.

preames reviewed Feb 2, 2024

View reviewed changes

MaskRay reviewed Feb 2, 2024

View reviewed changes

MatzeB reviewed Feb 2, 2024

View reviewed changes

slinder1 reviewed Feb 2, 2024

View reviewed changes

lld/ELF/ScriptParser.cpp Show resolved Hide resolved

[DRAFT][clang-format][lld] Run clang-format on lld #80461

Are you sure you want to change the base?

[DRAFT][clang-format][lld] Run clang-format on lld #80461

Uh oh!

Conversation

pogo59 commented Feb 2, 2024

Uh oh!

preames 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!

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!

Choose a reason for hiding this comment

Uh oh!

MatzeB Feb 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

MatzeB Feb 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MatzeB 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!

MatzeB Feb 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MatzeB Feb 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MatzeB Feb 3, 2024 •

edited

Loading

MatzeB Feb 2, 2024 •

edited

Loading

MatzeB Feb 2, 2024 •

edited

Loading

MatzeB Feb 2, 2024 •

edited

Loading