Skip to content

Conversation

@oschwald
Copy link
Member

No description provided.

This provides a small performance improvement.
The reason for doing this now is that the code was already untidy
according to uncrustify. Given this, I took the opportunity to switch to
clang-format given that is the direction we have been moving in other
repos.
@coveralls
Copy link

coveralls commented Sep 24, 2019

Coverage Status

Coverage increased (+2.02%) to 93.583% when pulling 3aac426 on greg/network into 17a155b on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.1%) to 90.421% when pulling 9bc207e on greg/network into 17a155b on master.

Copy link
Contributor

@horgh horgh left a comment

Choose a reason for hiding this comment

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

Great! Just a couple minor things I noticed.

};
static PyMethodDef Metadata_methods[] = {{NULL, NULL, 0, NULL}};

/* *INDENT-OFF* */
Copy link
Contributor

Choose a reason for hiding this comment

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

We could remove these comments I think?

static PyTypeObject Metadata_Type = {
PyVarObject_HEAD_INIT(NULL, 0)
.tp_basicsize = sizeof(Metadata_obj),
PyVarObject_HEAD_INIT(NULL, 0).tp_basicsize = sizeof(Metadata_obj),
Copy link
Contributor

Choose a reason for hiding this comment

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

Hrm, this isn't great. I saw one like this above (first file in the commit) as well, though not as weird.

PyErr_SetString(
PyExc_RuntimeError,
"getaddrinfo was successful but failed to set the addrinfo");
return 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch!

@horgh horgh merged commit 69d388c into master Sep 24, 2019
@horgh horgh deleted the greg/network branch September 24, 2019 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants