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

Feature/add proto attrs #2604

Closed
wants to merge 4 commits into from

Conversation

reyoung
Copy link
Collaborator

@reyoung reyoung commented Jun 26, 2017

No description provided.

Make paddle framework use Error as Error Type.
@reyoung
Copy link
Collaborator Author

reyoung commented Jun 26, 2017

CI error is because of protobuf's issue protocolbuffers/protobuf#2425

Also fix protobuf 3.1.0 error
* Depends on protobuf 3.1.0, not an alpha version
* Remove Wextra, because protobuf 3.1.0 don't pass GCC check
@@ -101,15 +101,16 @@ set(COMMON_FLAGS
-fPIC
-fno-omit-frame-pointer
-Wall
-Wextra
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why remove this check?

# go_extern will download extern go project.
# go_extern(target_name extern_source)
# go_extern(go_redis github.com/hoisie/redis)
function(go_extern TARGET_NAME)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why we need this function in generic.cmake?

endfunction(go_extern)


function(generate_protobuf_cpp SRCS HDRS)
Copy link
Collaborator

Choose a reason for hiding this comment

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

According to the documented convention at the beginning of this header file, what we need is proto_library. But the adding of proto_libary seems should be in a separate PR that fixes #2567.

@@ -14,19 +14,18 @@ limitations under the License. */

#pragma once

#include <glog/logging.h>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's try don't use the string typed error until we know we must use it. I had this idea because at least in @jacquesqiao 's PR #2629 (comment), we don't need this error type.

@@ -0,0 +1,8 @@
syntax="proto3";
Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand unit test source files are named something like xxx_test.cc, but what is xxx_test.proto for?

@@ -0,0 +1,192 @@
#pragma once
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't quite understand what goal all the code here is trying to achieve. It might be easier if there is a design doc like paddle/framework/variable.md for paddle/framework/variable.h.

@reyoung
Copy link
Collaborator Author

reyoung commented Jun 29, 2017

Closed, AttributeReader move to PR #2604

@reyoung reyoung closed this Jun 29, 2017
@reyoung reyoung deleted the feature/add_proto_attrs branch October 28, 2017 22:19
heavengate pushed a commit to heavengate/Paddle that referenced this pull request Aug 16, 2021
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