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

[infra] protobuf -> src/protobuf #407

Merged
merged 1 commit into from
May 15, 2018
Merged

[infra] protobuf -> src/protobuf #407

merged 1 commit into from
May 15, 2018

Conversation

stonier
Copy link
Collaborator

@stonier stonier commented May 11, 2018

Also cleans up the protobuf descriptors generator (cmake) and makes
sure the .proto files are installed.

Part of #296

Also cleans up the protobuf descriptors generator (cmake) and makes
sure the .proto files are installed.
@stonier stonier added this to the Milestone #2 milestone May 11, 2018
@stonier stonier requested a review from caguero May 11, 2018 22:27
#include <pybind11/pybind11.h>

#include <Python.h>

#include "backend/automotive_simulator.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

protobuf and backend are directories located at the same level in the install folder. Looks a bit odd to me that we include them with different prefixes (see includes from lines 22, 23, 24 and lines 26, 27). One group has the delphyne prefix and not the other.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These, at the moment, are private headers. Usually I'd just include them as `#include "automotive_simulator.h", but cpplint won't let you do that (google style guide, ugh).

I totally agree, it's confusing ... unless you have a typical bazel package layout.

I'm inclined to eventually move these over to the include directory, regardless of whether they are private or not, but will do that when I do the headers / targets / include_directories overhaul.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good!

@caguero
Copy link
Contributor

caguero commented May 14, 2018

Looks good to me, just a minor comment about the way we're including headers.

@caguero caguero force-pushed the stonier/protobuf branch from 84965ac to b10b36c Compare May 15, 2018 00:24
@stonier stonier merged commit a88ffe9 into master May 15, 2018
@stonier stonier mentioned this pull request May 15, 2018
4 tasks
@stonier stonier deleted the stonier/protobuf branch May 24, 2018 02:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants