Skip to content

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

@reyoung reyoung force-pushed the feature/add_proto_attrs branch from ee42af8 to a4e4d19 Compare June 27, 2017 07:58
@reyoung reyoung force-pushed the feature/add_proto_attrs branch from a4e4d19 to e815fe2 Compare June 27, 2017 08:04
reyoung added 2 commits June 27, 2017 16:42
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
@reyoung reyoung force-pushed the feature/add_proto_attrs branch from 7d3271a to deeef35 Compare June 27, 2017 12:38
-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.

2 participants