DRAFT: First pass at a clang formatter by emily-howell · Pull Request #1500 · Alpine-DAV/ascent

emily-howell · 2025-04-15T17:56:05Z

Can be played around with locally by calling:

find ./src/libs/ascent/ -regex '.*\.\(c\|cpp\|h\|hpp\)' -exec clang-format -i {} +

I don't know that I want all of the formatting to go in in one big MR like this. This is more a vessel to visualize the types of changes it will make

…ntation

emily-howell · 2025-04-15T19:01:07Z

@cyrush @nicolemarsaglia this is a first pass a what formatting could look like. My changes are the .clang-format file. Everything else is the result of running the formatter on Ascent code. The goal here is not to merge this in but rather show what changes would occur if this was added.

Please criticize and tear this apart!! The goal here is to spur discussion if it is needed.

Eventually, this would need to be built into the CI and also be run on the tests and other files.

cyrush · 2025-04-16T22:52:02Z

src/libs/ascent/hola/ascent_hola.hpp

-
-#include <ascent_config.h>
-#include <ascent_exports.h>
+#    define ASCENT_HOLA_HPP


In general I like intended defs, however for the header file case there is always a indef / define that wraps the whole file to avoid multiple definitions.

It would be good to not indent the normal includes for this case.

Unfortunately, there isn't a ton of knobs to turn with respect to the preprocessor directives. The only thing I really found was to indent or not to indent. I personally really like the indentation as it greatly improves readability but I do agree that having a blanket policy for it means that some things have slightly wonky indentation.

cyrush · 2025-04-16T22:52:38Z

src/libs/ascent/python/ascent_module/ascent_python_exports.h

+#    else
+#        define ASCENT_PYTHON_API __declspec(dllimport)
+#    endif
+#    if defined(_MSC_VER)


indents do improve readability here

cyrush · 2025-04-16T22:53:42Z

src/libs/ascent/runtimes/ascent_data_object.hpp

 //-----------------------------------------------------------------------------
 // forward declare
-#if defined(ASCENT_DRAY_ENABLED)
+#    if defined(ASCENT_DRAY_ENABLED)


def indent nesting doesn't work well for this case

The setting Emily has now indents after the hash. There is another setting to indent the hash too. Is that what you didn't like, or you just didn't want it indented at all?

All our header files look like:

#ifndef HDR_FILE_NAME #define HDR_FILE_NAME ... #include <myheader1> #include <myheader2> #include <myheader3> #include <myheader4> ... #endif

The current setup transforms them all to:

#ifndef HDR_FILE_NAME # define HDR_FILE_NAME # include <myheader1> # include <myheader2> # include <myheader3> ... #endif

Not a fan of this, I don't think it is an improvement.

in this case HDR_FILE_NAME is to avoid duplicate defs, but its not otherwise important to the structure or semantics of the header file

cyrush · 2025-04-16T22:54:23Z

src/libs/ascent/runtimes/ascent_empty_runtime.cpp

-{
-    out.set(m_info);
-}
+void EmptyRuntime::Info(conduit::Node &out) { out.set(m_info); }


I prefer multi line even for these simple guys

Updated to only allow single line for empty functions

cyrush · 2025-04-16T22:55:28Z

src/libs/ascent/runtimes/ascent_empty_runtime.cpp

-    conduit::relay::mpi::sum_all_reduce(n_src,
-                                        n_reduce,
-                                        mpi_comm);
+    conduit::relay::mpi::sum_all_reduce(n_src, n_reduce, mpi_comm);


I also like multi line in these cases, not sure if it is possible to retain them.
For longer functions multi line allows you to add comments after each of the arguments.

If you add comments it won't put them on the same line anymore.

Yikes! I rely on that many places in Conduit: https://github.com/LLNL/conduit/blob/8f5a32ae6540eaead82d287f231fe84d0cd8540a/src/libs/relay/conduit_relay_io_silo.cpp#L4854

^ It would support that case Justin

The comments make it wrap, even on short lines

If you add comments it won't put them on the same line anymore.

I'm confused. So comments will be on the same line?

Oh I get it! When you said "them" you meant the arguments, not the comments for those arguments. 🙃

Yes, sorry, that wasn't clear

It will absolutely respect that comments are placed next to the arguments and it will wrap all of the arguments onto their own lines.

No worries. I was reading too fast.

.clang-format

JustinPrivitera · 2025-04-17T20:36:59Z

.clang-format

+AlignConsecutiveAssignments: false # Don't bother lining up `=` symbols in consecutive lines
+AlignConsecutiveDeclarations: false # Don't bother lining up `=` symbols in consecutive lines


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;

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

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

JustinPrivitera · 2025-04-17T20:39:10Z

.clang-format

+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


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.

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);

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

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

JustinPrivitera · 2025-04-17T20:40:43Z

.clang-format

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


Suggested change

IndentPPDirectives: AfterHash # don't indent things like `#ifdef` and `#define` unless nested

IndentPPDirectives: Always # don't indent things like `#ifdef` and `#define` unless nested

I prefer Always if we have to do this at all. With extreme nesting this can potentially be burdensome.

Always isn't an option but I can change it to BeforeHash which will also indent the hash.

Hah! Thanks Livchat. BeforeHash is the right answer then :)

JustinPrivitera · 2025-04-17T20:43:14Z

.clang-format

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


Suggested change

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

AccessModifierOffset: 0 # Access modifiers (public:, private:, protected:) should be inset

🙃

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();

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

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 :(.

emily-howell added 3 commits April 15, 2025 09:40

FIrst pass at a clang-format file

88fc726

Fixing includes sorting, PPDirectives indentations, and adding docume…

3523ffd

…ntation

Massive commit of the formatting produced by running clang-format

fec348b

emily-howell self-assigned this Apr 15, 2025

emily-howell requested review from cyrush and nicolemarsaglia April 15, 2025 18:57

cyrush reviewed Apr 16, 2025

View reviewed changes

emily-howell requested a review from JustinPrivitera April 17, 2025 16:16

JustinPrivitera reviewed Apr 17, 2025

View reviewed changes

emily-howell added 2 commits April 22, 2025 11:41

Updating the formatter settings

f111422

Applying updated clang format settings to code

95baba7

		AlignConsecutiveAssignments: false # Don't bother lining up `=` symbols in consecutive lines
		AlignConsecutiveDeclarations: false # Don't bother lining up `=` symbols in consecutive lines

	AllowAllParametersOfDeclarationOnNextLine: false
	AllowAllParametersOfDeclarationOnNextLine: true

	IndentPPDirectives: AfterHash # don't indent things like `#ifdef` and `#define` unless nested
	IndentPPDirectives: Always # don't indent things like `#ifdef` and `#define` unless nested

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

Conversation

emily-howell commented Apr 15, 2025

Uh oh!

emily-howell commented Apr 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emily-howell Apr 22, 2025 • 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

emily-howell commented Apr 15, 2025 •

edited

Loading

emily-howell Apr 22, 2025 •

edited

Loading