[Not For Landing] Main Wholesale reformatting discussion #3413

pmatos · 2024-02-07T15:45:52Z

This is related to #3412

It contains a full tree format, this is formatting:

FEXCore
FEXHeaderUtils
include
Scripts
Source
ThunkLibs

It's formatting using a clang-format calculated through clang-unformat from the FEXCore directory - i.e. specifically trying to minimize changes there.

Let me know what you think. We can use this PR to get suggestions on formatting improvements, I can then tweak the clang-format and push new versions of the tree until we are happy.

lioncash

Largely this is very nice! Just a few quick observations.

Also there's a few tables in OpcodeDispatcher that should probably be wrapped in format disabling comments, but I can't actually inline comment on them, because the file is so big that trying to open it locks up the browser tab

FEXCore/Source/Common/JitSymbols.cpp

FEXCore/Source/Common/StringConv.h

lioncash · 2024-02-07T15:56:07Z

FEXCore/Source/Common/BitSet.h

-  void MemSet(size_t Elements) {
-    memset(Memory, 0xFF, AlignUp(Elements / MinimumSizeBits, MinimumSizeBits));
-  }
+  bool Get(T Element) { return (Memory[Element / MinimumSizeBits] & (1ULL << (Element % MinimumSizeBits))) != 0; }


Maybe single line functions should be formatted the same as multi-line functions. Keeps it consistent, and leads to less visual noise when everything is clumped together like this.

Given the current column limit is set to 140, I believe functions this large will stay single line. Your example is already single line in commit c2a9b99

How about AllowShortFunctionsOnASingleLine: None/Empty ?

We currently have:
AllowShortFunctionsOnASingleLine: All
which is what allows functions in a single line. I assume this is what you want?

@lioncash or were you saying we should never allow functions in the same line? Or maybe just empty ones?

Personally I would allow empty ones only, since due to our 140 column limit, allowing anything makes things sometimes hard to read. What do others think? Maybe I misunderstood @lioncash initial comments.

I think allowing empty ones would be a good compromise (I had no idea that was an available sub-option, I thought it was just a straight-up yes/no toggle).

OK - So I will allow only empty ones to be collapsed into a single line with AllowShortFunctionsOnASingleLine: Emtpy.

FEXCore/Source/Interface/Core/JIT/Arm64/JITClass.h

FEXCore/Source/Interface/Core/ObjectCache/CodeObjectSerializationConfig.h

neobrain

After a quick first glance, I like it a lot! There seem to be some inconsistencies in the formatting - maybe a result of clang-unformat overfitting the settings a little? Definitely a big improvement overall though. Other than the obviously inconsistent stuff, I've added few wishlist items, but it's better to wait for others to weigh in before making any changes here.

I do wonder if setting the character limit to a generous 140 is doing more harm than good. You can see a lot of places that were neatly manually split into manual lines before turning into massive blobs. I'm not going to suggest a conservative 80, but maybe 100 is good enough? There's definitely places that are going to suffer from that, so I'm not sure what the right balance is.

neobrain · 2024-02-07T15:55:24Z

FEXCore/Source/Interface/Core/OpcodeDispatcher/Crypto.cpp

+  const auto Sigma1 = [this](OrderedNode *W) -> OrderedNode *
+  {


Can we make positioning of '{' for lambdas consistent with normal functions? Personal preference is to have it all on the same line.

It looks as you request in my current version. :)

neobrain · 2024-02-07T15:57:12Z

FEXCore/Source/Interface/HLE/Thunks/Thunks.cpp

+  "jmpq *0f(%rip) \n"
+  ".align 8 \n"
+  "0: \n"
+  ".quad 0, 0, 0, 0 \n" // TrampolineInstanceInfo


Not a fan of the lack of indentation here. Can we add one or two indentation levels here, i.e. 2 or 4 spaces?

Currently I managed that by tweaking ContinuationIndentWidth: 2, so anything that continues a previous expression on a different line is indented 2. Might of course have further consequences.

neobrain · 2024-02-07T15:57:39Z

FEXCore/Source/Interface/HLE/Thunks/Thunks.cpp

+  std::shared_mutex ThunksMutex;
+
+  fextl::unordered_map<IR::SHA256Sum, ThunkedFunction*, TruncatingSHA256Hash> Thunks = {
+  {// sha256(fex:loadlib)


Would prefer one/two indentation levels here as well, since it's a statement split across multiple lines.

I think that looks ok atm (in the current changes I will commit soon).

neobrain · 2024-02-07T16:00:12Z

FEXCore/Source/Interface/HLE/Thunks/Thunks.cpp


-    /**
+/**


clang-format un-indented only the first line of this docstring. Looks like we'll need to move the other lines manually :(

Solved as well.

neobrain · 2024-02-07T16:02:46Z

FEXCore/Source/Interface/IR/AOTIR.cpp

@@ -22,399 +22,383 @@


 namespace FEXCore::IR {
-  AOTIRInlineEntry *AOTIRInlineIndex::GetInlineEntry(uint64_t DataOffset) {
-    uintptr_t This = (uintptr_t)this;
+AOTIRInlineEntry *AOTIRInlineIndex::GetInlineEntry(uint64_t DataOffset) {


Seems like the star/ampersand placement is very irregular. It's using AOTIRInlineEntry * here (space between type and star) but auto& [Name, rv] = *reinterpret_cast<ArgsRV_t*>(ArgsRV);/HostToGuestTrampolinePtr* TrampolineAddress in Thunks.cpp. Any idea why it's like that?

I can see why it might choose not to put a space when the type is used in isolation (but then it's not doing that below for the (AOTIRInlineEntry *) cast). Also auto &[Name, rv] would look kind of weird, though I could get used to it. But everything else should be more consistent.

Setting PointerAlignment and ReferenceAlignment to Left aids with consistency, removed DerivePointerAlignment that was causing inconsistencies.

neobrain · 2024-02-07T16:13:01Z

Source/Tools/CommonTools/Linux/Utils/ELFContainer.cpp

@@ -69,7 +65,7 @@ ELFContainer::ELFType ELFContainer::GetELFType(int FD) {
  // We can't use dup since that makes the FD have the same underlying state backing both FDs.

  // We need to first determine the file size through fstat.
-  struct stat buf{};
+  struct stat buf {};


It's adding a space here, but down below it uses const char *RawString{}; (no space before {}).

The difference here is that the {} after const char *RawString are parsed as a Cpp11BracedList and therefore affected by SpaceBeforeCpp11BracedList. Setting this option to true, adds a space after RawString.

neobrain · 2024-02-07T16:22:19Z

FEXCore/Source/Interface/Core/ObjectCache/ObjectCacheService.cpp

-    AddressToEntryMap.clear();
-    UnrelocatedAddressToEntryMap.clear();
+  // Do final code region closures on thread shutdown
+  for (auto &it : AddressToEntryMap) {


auto &it is used here, but I've seen auto& var elsewhere.

After latest changes to alignment of references and pointers, everything is to the left.

pmatos · 2024-02-08T15:16:57Z

Thanks for all the amazing comments until now. I will play a bit with the setup, create a new clang-format and regenerate the tree for further analysis.

pmatos · 2024-02-12T08:10:09Z

I have been pushing changes over the old ones as I modify the clang-format file, but formatting changes done over other formatting changes don't reflect the result of doing the changes in one go. I will have to reformat the tree and push-force to see the results of each change.

pmatos · 2024-02-12T11:10:30Z

With the exception of the clang-format on/off in OpcodeDispatcher and X86Tables tables, I think I have addressed all the comments to the best of my knowledge. Any further comments/opinions/suggestions about the current formatting?

Sonicadvance1 · 2024-02-13T01:31:48Z

One thing to note is that we should make sure not to reformat code that isn't ours. FEXCore/Source/Common/SoftFloat-3e isn't our code for example.

neobrain · 2024-02-15T14:54:18Z

With the exception of the clang-format on/off in OpcodeDispatcher and X86Tables tables, I think I have addressed all the comments to the best of my knowledge. Any further comments/opinions/suggestions about the current formatting?

Great work! I've been casually browsing over it a few times more and nothing else caught my eye so far.

A personal note, I'd like to check how the clang AST matchers used in unittests/ThunkLibs/generator.cpp are formatted now. The readability of matching expressions is very sensitive to reformats, and disabling formatting locally for each of them would add too much noise to each test. We may just need to disable clang-format entirely for this file (though maybe we can do a manual, selective reformat once).

pmatos · 2024-02-22T07:59:44Z

One thing to note is that we should make sure not to reformat code that isn't ours. FEXCore/Source/Common/SoftFloat-3e isn't our code for example.

Uff, I expected all code outside external to be ours. I had never seen that directory before. Are there anymore like these outside external I should be aware of?

pmatos · 2024-02-22T08:38:42Z

One thing to note is that we should make sure not to reformat code that isn't ours. FEXCore/Source/Common/SoftFloat-3e isn't our code for example.

Uff, I expected all code outside external to be ours. I had never seen that directory before. Are there anymore like these outside external I should be aware of?

It looks like Source/Common/cpp-optparse is another example of those.

Sonicadvance1 · 2024-02-22T09:25:17Z

One thing to note is that we should make sure not to reformat code that isn't ours. FEXCore/Source/Common/SoftFloat-3e isn't our code for example.

Uff, I expected all code outside external to be ours. I had never seen that directory before. Are there anymore like these outside external I should be aware of?

It looks like Source/Common/cpp-optparse is another example of those.

I think you got the only two that I remember.

pmatos · 2024-02-22T09:29:25Z

One thing to note is that we should make sure not to reformat code that isn't ours. FEXCore/Source/Common/SoftFloat-3e isn't our code for example.

Uff, I expected all code outside external to be ours. I had never seen that directory before. Are there anymore like these outside external I should be aware of?

It looks like Source/Common/cpp-optparse is another example of those.

A .clang-format-ignore file support will be added in clang-18.1.0 (llvm/llvm-project@0930812) but we'll have to maybe work around this until we can use this on our CI.

neobrain · 2024-02-22T09:47:54Z

One thing to note is that we should make sure not to reformat code that isn't ours. FEXCore/Source/Common/SoftFloat-3e isn't our code for example.

If there's an easy way to avoid reformatting of extern code, let's go for it.

If not, I'd be willing to gloss over this issue considering we're talking about forked code after all. Synchronizing with upstream should still be easy since you can just apply our formatting style to upstream before diffing.

As a middleground solution, maybe just adding a single // clang-format off at the top of each affected file is sufficient?

neobrain · 2024-02-22T09:54:02Z

A personal note, I'd like to check how the clang AST matchers used in unittests/ThunkLibs/generator.cpp are formatted now. The readability of matching expressions is very sensitive to reformats, and disabling formatting locally for each of them would add too much noise to each test. We may just need to disable clang-format entirely for this file (though maybe we can do a manual, selective reformat once).

I took a look at this: Nothing atrocious, however there are some weird effects that don't seem intentional.

Fortunately, the clang AST matcher expressions themselves are formatted fine.

This one is a bit weird though:

    CHECK_THAT(
      output,
      matches(classTemplateSpecializationDecl(
        hasName("guest_layout"), hasAnyTemplateArgument(refersToType(asString("struct A"))),
        // The member "data" exists and is defined to a struct...
        has(fieldDecl(
          hasName("data"),
          hasType(hasCanonicalType(hasDeclaration(decl(
            // ... the members of which also use guest_layout (with fixed-size integers)
            has(fieldDecl(hasName("a"), hasType(asString("guest_layout<int32_t>")))), has(fieldDecl(hasName("b"), hasType(asString("guest_"
                                                                                                                                   "layout<"
                                                                                                                                   "int32_"
                                                                                                                                   "t"
                                                                                                                                   ">")))))))))))));

Why does it decide to break up the string several times instead of putting any of the has(...) expressions on a new line?

Also, the runInvocation() signature gets re-formatted into a single line, overstepping the column limit of 140 characters a lot. Any idea what's going on there?

pmatos · 2024-02-22T10:32:43Z

I have just pushed a new change. This one does several things:

Adds a wrapper to clang-format that takes .clang-format-ignore into consideration. It should be easy to use this in our editors instead of the system one. Also CI can use this directly.
Adds the .clang-format-ignore for ease of adding things to ignore.
Reformatted the tree.

lioncash

Doing a quick scan through some files, and this looks quite nice

I think we're missing: AllowShortFunctionsOnASingleLine: Empty from previous discussion from being applied, but aside from that, I have no remaining nits =)

pmatos · 2024-02-22T10:51:28Z

Also, the runInvocation() signature gets re-formatted into a single line, overstepping the column limit of 140 characters a lot. Any idea what's going on there?

oh my - you're right. It looks like a bug in clang-format. The new line has something like 195 characters. I will look into it.

pmatos · 2024-02-22T10:52:19Z

Doing a quick scan through some files, and this looks quite nice

I think we're missing: AllowShortFunctionsOnASingleLine: Empty from previous discussion from being applied, but aside from that, I have no remaining nits =)

I think you're right, sorry. I will look into the issue @neobrain pointed out and then re-run a format taking that into consideration.

neobrain · 2024-02-22T14:35:13Z

unittests/FEXLinuxTests/tests/signal/invalid_hlt.cpp

@@ -12,14 +12,13 @@
 extern "C" void IntInstruction();

 #pragma GCC diagnostic ignored "-Wattributes" // Suppress warning in case control-flow checks aren't enabled
-__attribute__((naked, nocf_check))
-  static void InvalidINT() {
+__attribute__((naked, nocf_check)) static void InvalidINT() {


Should we enable BreakAfterAttributes?

CC @lioncash , since this seems analogous to breaking after template<>.

If we do this, we may want to add these macros to AttributeMacros: JEMALLOC_NOTHROW, FEX_ALIGNED, FEX_ANNOTATE, FEX_DEFAULT_VISIBILITY, FEX_NAKED, FEX_PACKED, FEXCORE_PRESERVE_ALL_ATTR, GLIBC_ALIAS_FUNCTION

neobrain · 2024-02-22T14:39:28Z

unittests/ThunkLibs/generator.cpp

+            has(fieldDecl(hasName("a"), hasType(asString("guest_layout<int32_t>")))), has(fieldDecl(hasName("b"), hasType(asString("guest_"
+                                                                                                                                   "layout<"
+                                                                                                                                   "int32_"
+                                                                                                                                   "t"
+                                                                                                                                   ">")))))))))))));


Following up on #3413 (comment):

Why does it decide to break up the string several times instead of putting any of the has(...) expressions on a new line?

PenaltyBreakString: 10 (or higher) will fix this, but I haven't checked what ripple effects that has on the rest of FEX. Seems highly desirable in this particular instance though. (Note that clang-format won't un-break strings, so changes here need to be re-applied on the pre-format sources).

I have removed the option and compared the trees with and without the option side by side and it seems that there's no situation where that option actually makes things better.

The only different I see that's more relevant are things like (in eflags_signal.cpp) for example:

I will regenerate the tree again.

pmatos · 2024-03-22T09:58:45Z

There has not been any comments as of late on this. Here's my suggestion:

I will work on CI integration in a different PR.
Choose a day for the merge of the reformatting.
On that day, we commit:

the reformatting, then
.clang-format file and
the ignore blame list file (which contains the sha of the reformatting commit)
and finally the CI integration PR.

What do you think?

Sonicadvance1 · 2024-03-22T10:09:42Z

There has not been any comments as of late on this. Here's my suggestion:

I will work on CI integration in a different PR.

Choose a day for the merge of the reformatting.

On that day, we commit:

the reformatting, then

.clang-format file and

the ignore blame list file (which contains the sha of the reformatting commit)

and finally the CI integration PR.

What do you think?

Sounds reasonable to me.

neobrain · 2024-04-01T11:33:54Z

What do you think?

Yes, really happy with how the formatting turned out - let's move forward with it :)

pmatos · 2024-04-08T08:43:19Z

There has not been any comments as of late on this. Here's my suggestion:

I will work on CI integration in a different PR.

Choose a day for the merge of the reformatting.

On that day, we commit:

the reformatting, then

.clang-format file and

the ignore blame list file (which contains the sha of the reformatting commit)

and finally the CI integration PR.

What do you think?

If we agree on merging Friday April 12th at around 9am CEST (midnight PDT). Happy to hear other suggestions for merge timings.

I can prepare a PR with the reformatting of the source code end of Thursday PDT time, so that there are no commits between the last merged PR and creation of the reformatting commit.
Once the reformatting is merged, I can prepare straight away the .clang-format PR with the blamelist sha (which I can only obtain after reformatting lands).
Then we can land CI workflow to check clang-format usage on pull requests #3561.

alyssarosenzweig · 2024-04-08T11:32:33Z

We probably want to prioritize reviewing & landing as much as possible this week then, else the conflicts are going to stink. (There are ways around this but it's annoying.)

pmatos · 2024-04-08T12:08:22Z

We probably want to prioritize reviewing & landing as much as possible this week then, else the conflicts are going to stink. (There are ways around this but it's annoying.)

Yes - that makes sense. We can always try to do that this week. If it doesn't work, we can push this plan to next week instead. I think there's not a lot of pressure on this. :)

This follows discussions from FEX-Emu#3413. Followup commits add clang-format file, script and blame ignore lists.

pmatos · 2024-04-12T08:32:04Z

As mentioned in discord:

I added in #3572, a script to help reformat given a clang-format-ignore (cause there are files we don't want to reformat).
For people working on branches, before rebasing on main, it's easier to reformat the whole tree first locally.
This should reduce conflicts.
Once the massive refactor is landed I can add another PR to ensure that this commit is ignore in git blames but I need it's sha first.
So there'll be 4 commits in total, in landing order:

Reformat Whole-tree reformat #3571
git blame ignore - to come
clang-format file Clang Format file and script #3572
CI workflow CI workflow to check clang-format usage on pull requests #3561

Once #3571 lands, get the files from #3572 and run on your local branches:
find . -iname '*.h' -o -iname '*.cpp' -exec python3 Scripts/clang-format.py -i \{\} \;

This should help with conflicts. Make sure you have a recent clang-format.

This follows discussions from FEX-Emu#3413. Followup commits add clang-format file, script and blame ignore lists.

pmatos · 2024-04-13T06:45:56Z

Reformat has landed - so gitblame ignore revs file is in #3575 .

pmatos · 2024-04-16T12:39:02Z

The last PR from this work was just landed. Thank you all for the discussion, reviewing and landing the PRs. Closing this.

pmatos mentioned this pull request Feb 7, 2024

Clang Format file proposal #3412

Closed

lioncash reviewed Feb 7, 2024

View reviewed changes

FEXCore/Source/Interface/Core/JIT/Arm64/JITClass.h Outdated Show resolved Hide resolved

FEXCore/Source/Interface/Core/JIT/Arm64/JITClass.h Outdated Show resolved Hide resolved

FEXCore/Source/Interface/Core/ObjectCache/CodeObjectSerializationConfig.h Outdated Show resolved Hide resolved

neobrain reviewed Feb 7, 2024

View reviewed changes

pmatos force-pushed the ClangFormatFEXCore branch from c2a9b99 to 212a59e Compare February 12, 2024 11:09

pmatos force-pushed the ClangFormatFEXCore branch from 212a59e to 7a0d707 Compare February 22, 2024 10:26

lioncash reviewed Feb 22, 2024

View reviewed changes

neobrain reviewed Feb 22, 2024

View reviewed changes

pmatos force-pushed the ClangFormatFEXCore branch from 7a0d707 to 54cadb1 Compare February 22, 2024 17:54

pmatos added 3 commits February 23, 2024 09:29

Main formatting files

f98e83c

Add clang-format wrapper script

c563701

Wholesale reformat

bda4fa5

pmatos force-pushed the ClangFormatFEXCore branch from 54cadb1 to bda4fa5 Compare February 23, 2024 09:29

pmatos added a commit to pmatos/FEX that referenced this pull request Apr 12, 2024

Whole-tree reformat

420d937

This follows discussions from FEX-Emu#3413. Followup commits add clang-format file, script and blame ignore lists.

pmatos mentioned this pull request Apr 12, 2024

Whole-tree reformat #3571

Merged

pmatos changed the title ~~Formatted with FEXCore clang-format~~ [Not For Landing] Main Wholesale reformatting discussion Apr 12, 2024

pmatos marked this pull request as draft April 12, 2024 08:32

pmatos added a commit to pmatos/FEX that referenced this pull request Apr 12, 2024

Whole-tree reformat

b6834ea

This follows discussions from FEX-Emu#3413. Followup commits add clang-format file, script and blame ignore lists.

pmatos added a commit to pmatos/FEX that referenced this pull request Apr 12, 2024

Whole-tree reformat

2b4ec88

This follows discussions from FEX-Emu#3413. Followup commits add clang-format file, script and blame ignore lists.

pmatos closed this Apr 16, 2024

pmatos deleted the ClangFormatFEXCore branch April 16, 2024 12:39

		const auto Sigma1 = [this](OrderedNode W) -> OrderedNode
		{

[Not For Landing] Main Wholesale reformatting discussion #3413

[Not For Landing] Main Wholesale reformatting discussion #3413

Conversation

pmatos commented Feb 7, 2024

lioncash left a comment • edited Loading

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

lioncash Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neobrain left a comment

Choose a reason for hiding this comment

neobrain Feb 7, 2024 • edited Loading

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

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

pmatos commented Feb 8, 2024

pmatos commented Feb 12, 2024

pmatos commented Feb 12, 2024

Sonicadvance1 commented Feb 13, 2024

neobrain commented Feb 15, 2024 • edited Loading

pmatos commented Feb 22, 2024

pmatos commented Feb 22, 2024

Sonicadvance1 commented Feb 22, 2024

pmatos commented Feb 22, 2024

neobrain commented Feb 22, 2024 • edited Loading

neobrain commented Feb 22, 2024 • edited Loading

pmatos commented Feb 22, 2024

lioncash left a comment

Choose a reason for hiding this comment

pmatos commented Feb 22, 2024 • edited Loading

pmatos commented Feb 22, 2024

Choose a reason for hiding this comment

neobrain Feb 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmatos commented Mar 22, 2024

Sonicadvance1 commented Mar 22, 2024

neobrain commented Apr 1, 2024

pmatos commented Apr 8, 2024

alyssarosenzweig commented Apr 8, 2024

pmatos commented Apr 8, 2024

pmatos commented Apr 12, 2024

pmatos commented Apr 13, 2024

pmatos commented Apr 16, 2024

lioncash left a comment •

edited

Loading

lioncash Feb 12, 2024 •

edited

Loading

neobrain Feb 7, 2024 •

edited

Loading

neobrain commented Feb 15, 2024 •

edited

Loading

neobrain commented Feb 22, 2024 •

edited

Loading

neobrain commented Feb 22, 2024 •

edited

Loading

pmatos commented Feb 22, 2024 •

edited

Loading

neobrain Feb 22, 2024 •

edited

Loading