Skip to content
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.

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

Add clang-format #291

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Add clang-format #291

wants to merge 10 commits into from

Conversation

krlmlr
Copy link
Member

@krlmlr krlmlr commented Nov 13, 2022

No description provided.

Copy link
Member Author

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

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

Thanks! I'd like to do a couple more iterations.

bool DbConnection::is_valid() {
return !!get_conn();
}
bool DbConnection::is_valid() { return !!get_conn(); }
Copy link
Member Author

Choose a reason for hiding this comment

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

I'd rather avoid braces on a single line.

_["thread.id"] = (int) mysql_thread_id(pConn_)
);
_["protocol.version"] = (int)mysql_get_proto_info(pConn_),
_["thread.id"] = (int)mysql_thread_id(pConn_));
Copy link
Member Author

Choose a reason for hiding this comment

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

Can we move the closing paren ); to the next line?

Copy link
Contributor

Choose a reason for hiding this comment

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

This feature will be added to clang-format. Seems now doesn't exists.


if (pCurrentResult_ != NULL) {
if (pResult != NULL)
warning("Cancelling previous query");
if (pResult != NULL) warning("Cancelling previous query");
Copy link
Member Author

Choose a reason for hiding this comment

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

I'd like to always add braces, but keep it on a new line.

const Nullable<std::string>& password,
const Nullable<std::string>& db, unsigned int port,
const Nullable<std::string>& unix_socket,
unsigned long client_flag, const Nullable<std::string>& groups,
Copy link
Member Author

Choose a reason for hiding this comment

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

Can we ensure that each argument gets its own line in these cases?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think now it's not possible. Clang-format add new line if arguments are to long, but it doesn't check the context of this lines.

case MY_INT64:
binding_update(j, MYSQL_TYPE_LONGLONG, 0);
break;
case MY_LGL:
Copy link
Member Author

Choose a reason for hiding this comment

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

The original indent was fine.

@@ -16,17 +17,17 @@ class MariaBinding : public boost::noncopyable {
std::vector<MariaFieldType> types;
std::vector<MYSQL_TIME> time_buffers;

public:
public:
Copy link
Member Author

Choose a reason for hiding this comment

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

Single-space indent for access modifiers looks awkward, but I'm willing to be convinced otherwise.

#include "MariaResultPrep.h"
#include "MariaResultSimple.h"

#include "pch.h"
Copy link
Member Author

Choose a reason for hiding this comment

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

Why did the "include" move? The pch.h include must come first here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Clang-format reordering header includes by some rules. But I think it's more general question. Seems pch.h is not "real" precompiled header, it's more like general include.

But it cause to problem with order of includes, so if order is changes then build is failing.

Maybe it is make sense to fix includes, that they do not depend on the order?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could specify IncludeCategories so that pch.h is always sorted first?

@Antonov548
Copy link
Contributor

Found useful tool to check configurations of clang-format: https://zed0.co.uk/clang-format-configurator/

@krlmlr
Copy link
Member Author

krlmlr commented Nov 28, 2022

Can we try https://clangpowertools.com/clang-format-editor.html on the main branch?

@krlmlr
Copy link
Member Author

krlmlr commented Nov 28, 2022

We also want clang-tidy so that we can, e.g., always add braces. Perhaps start with https://github.com/duckdb/duckdb/blob/master/.clang-tidy ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants