Skip to content

DRAFT: First pass at a clang formatter #1500

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
emily-howell wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

DRAFT: First pass at a clang formatter #1500

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
88fc726
FIrst pass at a clang-format file
emily-howell Apr 15, 2025
3523ffd
Fixing includes sorting, PPDirectives indentations, and adding docume…
emily-howell Apr 15, 2025
fec348b
Massive commit of the formatting produced by running clang-format
emily-howell Apr 15, 2025
f111422
Updating the formatter settings
emily-howell Apr 22, 2025
95baba7
Applying updated clang format settings to code
emily-howell Apr 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions .clang-format
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
---
### Run using:
### find ./src/libs/ascent/ -regex '.*\.\(c\|cpp\|h\|hpp\)' -exec clang-format -i {} +


#############################
### Alignment and Spacing ###
#############################

# Basic spacing and line length limits
ColumnLimit: 79
PenaltyExcessCharacter: 1000000 # Allows for strings to overflow past column
AlignArrayOfStructures: Left # Aligns columns of structures so they all line up clearly
AlignConsecutiveAssignments: false # Don't bother lining up `=` symbols in consecutive lines
AlignConsecutiveDeclarations: false # Don't bother lining up `=` symbols in consecutive lines
Comment on lines +14 to +15

@JustinPrivitera JustinPrivitera Apr 17, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this enforce this

int a = 1;
float b = 2.0;
double c = 3.0;

versus this?

int a    = 1;
float b  = 2.0;
double c = 3.0;

or does it just relax the restriction so users can do whatever?

Also, as far as I know, AlignConsecutiveDeclarations is not for lining up = symbols, it is aligning declarations like this:

int    a;
float  b;
double c;

@emily-howell emily-howell Apr 22, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AlignConsecutiveDeclarations will turn

int a = 1;
float bbb = 2.0;
double c = 3.0;

into

int    a   = 1;
float  bbb = 2.0;
double c   = 3.0;

And AlignConsecutiveAssignments applies to assignments both in declarations and not

a = 1;
bbb = 2.0;
c = 3.0;

into

a   = 1;
bbb = 2.0;
c   = 3.0;

It does not relax the requirement.

I personally don't want these to be set to true. You end up with a lot of changes like:

T identity = std::numeric_limits<T>::lowest();
const int size = accessor.m_size;

using for_policy = typename Exec::for_policy;
using reduce_policy = typename Exec::reduce_policy;

becoming

T         identity = std::numeric_limits<T>::lowest();
const int size     = accessor.m_size;

using for_policy_thing    = typename Exec::for_policy;
using reduce_policy_thing = typename Exec::reduce_policy;

Which looks wonky to me personally

@emily-howell emily-howell Apr 22, 2025 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does look like the default behavior is to not align things


# For lines that get wrapped, this forces one parameter per line and aligns them
BinPackParameters: false # One param per line if wrapped
BinPackArguments: false # One argument per line if wrapped
AlignAfterOpenBracket: Align # Align wrapped args/params under the first
AllowAllParametersOfDeclarationOnNextLine: false

@JustinPrivitera JustinPrivitera Apr 17, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
AllowAllParametersOfDeclarationOnNextLine: false
AllowAllParametersOfDeclarationOnNextLine: true

I think this should be true. Sometimes namespace::method() names get so long that it is useful to put parameter declarations on the following lines.

@emily-howell emily-howell Apr 22, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I don't like is that when you make that change you get:

conduit::Node ASCENT_API field_reduction_max(
    const conduit::Node &field, const std::string &component = "");

rather than when it is false it gives:

conduit::Node ASCENT_API
history_gradient_range(const conduit::Node &y_values,
                       const conduit::Node &dx_values);

@JustinPrivitera JustinPrivitera Apr 23, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it let you put the arguments on multiple lines when set to true?

@emily-howell emily-howell Apr 23, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does, but it seems to prioritize grouping things in a way I don't love. When there are enough arguments or the named are long enough both true and false will wrap the arguments onto their own lines


# Indentation
IndentWidth: 4
UseTab: Never
IndentPPDirectives: BeforeHash # don't indent things like `#ifdef` and `#define` unless nested


#################
### {} Braces ###
#################
BreakBeforeBraces: Allman # Braces get their own lines
SpaceBeforeParens: ControlStatements
AllowShortFunctionsOnASingleLine: Empty

################
### Includes ###
################
SortIncludes: false # Don't auto-sort #includes
IncludeBlocks: Preserve # Keeps include groups separate


##################
### Namespaces ###
##################
AccessModifierOffset: -4 # Access modifiers (public:, private:, protected:) should be inset

@JustinPrivitera JustinPrivitera Apr 17, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
AccessModifierOffset: -4 # Access modifiers (public:, private:, protected:) should be inset
AccessModifierOffset: 0 # Access modifiers (public:, private:, protected:) should be inset

🙃

@emily-howell emily-howell Apr 22, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. I don't know that I agree with this. That change would make header files significantly harder to read.

AccessModifierOffset: -4

class ASCENT_API ExpressionEval
{
protected:
    DataObject m_data_object;
    flow::Workspace w;
    static Cache m_cache;
    void jit_root(conduit::Node &root, const std::string &expr_name);

public:
    ExpressionEval(DataObject &dataset);
    ExpressionEval(conduit::Node *dataset);
    DataObject &data_object();
}

AccessModifierOffset: 0

class ASCENT_API ExpressionEval
{
    protected:
    DataObject m_data_object;
    flow::Workspace w;
    static Cache m_cache;
    void jit_root(conduit::Node &root, const std::string &expr_name);

    public:
    ExpressionEval(DataObject &dataset);
    ExpressionEval(conduit::Node *dataset);
    DataObject &data_object();

@JustinPrivitera JustinPrivitera Apr 23, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I blame Livchat again. I like the -4. I (mistakenly) thought -4 and 0 would have the opposite effect of what they actually have.

@emily-howell emily-howell Apr 23, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LivChat really does hallucinate a lot on this topic unfortunately. It is especially disappointing when I ask for a setting to change something and it makes up something only for that setting to not really exist :(.

...
Loading