Skip to content
This repository has been archived by the owner on Mar 8, 2020. It is now read-only.

Style fixes #22

Merged
merged 6 commits into from
Aug 31, 2017
Merged

Style fixes #22

merged 6 commits into from
Aug 31, 2017

Conversation

abeaumont
Copy link
Contributor

  • Make code follow Google C++ Style Guide
  • Simplified the API removing some unneeded functions
  • Renamed types, functions and variables to make the API more consistent and easier to understand.

Should solve #16

@abeaumont abeaumont requested a review from juanjux August 31, 2017 13:20
@abeaumont abeaumont self-assigned this Aug 31, 2017
* Remove unneded find context creation function from public API
* Remove interface getter from public API
* Rename types, variables and functions to:
 - Make them consistent
 - Make them more easily understandable (i.e. Nodes vs FindCtx)
#include "roles.h"

#include <stddef.h>

static const char *id_to_roles[] = {`)

for i := 0; i < int(lastRole); i++ {
Copy link
Contributor

@juanjux juanjux Aug 31, 2017

Choose a reason for hiding this comment

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

Lower Camel case (lastRole).

Copy link
Contributor Author

@abeaumont abeaumont Aug 31, 2017

Choose a reason for hiding this comment

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

This is Go code, not C.

Copy link
Contributor

Choose a reason for hiding this comment

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

True...

@@ -11,7 +11,7 @@ bool fail_xmlAddChild = false;
bool fail_xmlXPathNewContext = false;
Copy link
Contributor

@juanjux juanjux Aug 31, 2017

Choose a reason for hiding this comment

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

Lower Camel case and underline, but I guess this is because of the xml lib? (there are others below like xmlChar, xmlVersion, etc).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think in this case is a good idea to leave them as they're for readability reasons.

tests/main.cc Outdated
}

// add a suite to the registry
CU_pSuite pSuite = CU_add_suite("Suite_1", NULL, NULL);
Copy link
Contributor

@juanjux juanjux Aug 31, 2017

Choose a reason for hiding this comment

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

Lower Camel case (pSuite, also many others in this file).

return true;
}

void testUastNew() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Google conventions require functions to start with uppercase, but I don't know if here the testing framework defines the naming.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought so, but apparently not.

@abeaumont abeaumont merged commit 409ef13 into bblfsh:master Aug 31, 2017
@abeaumont abeaumont deleted the fix/code-style branch August 31, 2017 14:08
@abeaumont abeaumont mentioned this pull request Sep 1, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants