Opened 3 years ago

Closed 2 years ago

#13513 closed enhancement (fixed)

[BTRFS] Code refactoring and integrate with fs_shell

Reported by: hyche Owned by: nobody
Priority: normal Milestone: Unscheduled
Component: - General Version: R1/Development
Keywords: Cc:
Blocked By: Blocking:
Has a Patch: yes Platform: All

Description

Refactoring btrfs source:

  • Fixed style violation
  • System header files should be the first to be included

Also if FS_SHELL is defined, compile with fs_shell headers instead.

Attachments (5)

0001-Code-refactoring.patch (66.4 KB ) - added by hyche 3 years ago.
0001-Code-refactoring.2.patch (66.5 KB ) - added by hyche 3 years ago.
0001-Code-refactoring.3.patch (66.5 KB ) - added by hyche 3 years ago.
0001-Fixed-Code-style.patch (45.3 KB ) - added by hyche 3 years ago.
0002-Initialize-btrfs_shell.patch (13.1 KB ) - added by hyche 3 years ago.

Download all attachments as: .zip

Change History (14)

by hyche, 3 years ago

Attachment: 0001-Code-refactoring.patch added

comment:1 by hyche, 3 years ago

Has a Patch: set

in reply to:  description comment:2 by owenca, 3 years ago

Replying to hyche:

  • System header files should be the first to be included

Except that the CPP file's own header should be the first #include to ensure it's self-contained, which is the case with the headers in your patch.

From https://www.haiku-os.org/development/coding-guidelines, "the header that belongs to a source file should be included first to ensure self containment".

by hyche, 3 years ago

by hyche, 3 years ago

comment:3 by pulkomandy, 3 years ago

  • The asterisk style is not fixed (next to the type or next to the variable). As long as it is consistent within one file
  • The indentation in class declarations was correct before your changes. The idea is to have one column for modifiers (virtual, const, static, etc), one column for types, and one column for variables and function names.
  • There are always two blank lines between blocks (for example AttributeIterator.h line 12, between the includes and the class declaration - also in the other files)

If possible separate the style changes and fs_shell wrpaping in two commits. Also, you don't need to move <new> and <lib.h> if they are the same in both cases.

in reply to:  3 comment:4 by hyche, 3 years ago

Replying to pulkomandy:

  • The asterisk style is not fixed (next to the type or next to the variable). As long as it is consistent within one file

I used the tool "checkstyle.py", it told me reference/pointer should be next to type.

  • The indentation in class declarations was correct before your changes. The idea is to have one column for modifiers (virtual, const, static, etc), one column for types, and one column for variables and function names.
  • There are always two blank lines between blocks (for example AttributeIterator.h line 12, between the includes and the class declaration - also in the other files)

If possible separate the style changes and fs_shell wrpaping in two commits.

Will fix in next patches!

Also, you don't need to move <new> and <lib.h> if they are the same in both cases.

I need to move those 2 headers to make sure they are included after fs_shell wrapper.

comment:5 by pulkomandy, 3 years ago

I need to move those 2 headers to make sure they are included after fs_shell wrapper.

Ok, in that case a comment at the inclusion site may be useful:

// This needs to be included after the fs_shell wrapper
#include <zlib.h>

comment:6 by hyche, 3 years ago

I add more changes in btrfs_shell, It fixes the crash when run btrfs_shell on a device.

comment:7 by pulkomandy, 3 years ago

// This needs to be included after the fs_shell wrapper
#include <zlib.h>
#include <new>
    
#include "fssh_api_wrapper.h"

They appear to be included before the wrapper, now?

I'm not sure about the indentation style for inline methods braces:

    void    ThisLooksStrange {
                DoStuff(); }

I would stick with how it was before:

    void    ThisLooksBetter
                { DoStuff(); }

We should adjust the coding guidelines if it is not mentionned there, and the checkstyle tool if it detects it. The tool is not always right.

Maybe someone else can comment on the exact formatting to use in that case?

I think you can remove the libHaikuCompat thing in your jamfile (and in bfs_shell one too, if it's still there). This was used to run the tools on BeOS, but we don't support that anymore.

in reply to:  7 comment:8 by hyche, 3 years ago

Replying to pulkomandy:

// This needs to be included after the fs_shell wrapper
#include <zlib.h>
#include <new>
    
#include "fssh_api_wrapper.h"

They appear to be included before the wrapper, now?

After something mean it will be done first and then something does, right ? I'm confused now.

I'm not sure about the indentation style for inline methods braces:

    void    ThisLooksStrange {
                DoStuff(); }

I would stick with how it was before:

    void    ThisLooksBetter
                { DoStuff(); }

There are 2 styles like this in the code base, I just chose one and rewrite. As you can see here and here

I think you can remove the libHaikuCompat thing in your jamfile (and in bfs_shell one too, if it's still there). This was used to run the tools on BeOS, but we don't support that anymore.

Got it!

Version 1, edited 3 years ago by hyche (previous) (next) (diff)

by hyche, 3 years ago

Attachment: 0001-Fixed-Code-style.patch added

comment:9 by pulkomandy, 2 years ago

Resolution: fixed
Status: newclosed

Applied in hrev51282, sorry for the delay. You can submit more things for upstreaming, if more parts of your code are ready. It's ok to open a ticket and point to specific commits on your github fork, no need to submit them as patches. But the ticket is important as it helps tracking what to merge.

Note: See TracTickets for help on using tickets.