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

Source code refactoring. Extracted common functionality in cpp_helpers.c... #128

Merged
merged 6 commits into from Dec 12, 2014

Conversation

@krishnanm86
Copy link
Contributor

krishnanm86 commented Dec 10, 2014

...c and python/python_generator.cc

krinara86 added 2 commits Dec 10, 2014
…s.cc and python/python_generator.cc
@googlebot

This comment has been minimized.

Copy link

googlebot commented Dec 10, 2014

CLAs look good, thanks!

@googlebot googlebot added the cla: yes label Dec 10, 2014
@@ -360,21 +360,31 @@ string FilenameIdentifier(const string& filename) {
return result;
}


string GlobalSymbolName(const string& filename, string prefix)

This comment has been minimized.

Copy link
@xfxyjwf

xfxyjwf Dec 10, 2014

Contributor

Please revert changes to this file. I don't think this refactoring helps.

"class_name", descriptor.name());

void Generator::PrintDescriptorKeyAndModuleName(const ServiceDescriptor& descriptor, const char* keyandmodule) const {
printer_->Print(keyandmodule,"class_name", descriptor.name());

This comment has been minimized.

Copy link
@xfxyjwf

xfxyjwf Dec 10, 2014

Contributor

Put this first print statement back to PrintServiceClass() and PrintServiceStub(). The result should be like:
void Generator::PrintServiceClass(...) {
printer->Print("$class_name$ = ..."
"...."
"class_name", descriptor.name());
printer->Indent();
PrintDescriptorKeyAndModuleName(descriptor);
printer->Print("))\n\n");
printer->Outdent();
}

@krishnanm86

This comment has been minimized.

Copy link
Contributor Author

krishnanm86 commented Dec 11, 2014

Implemented the feedback. Hope we can land it now.

Anyway could I ask you feedback on the merge which you wanted to revert. I am collecting data on the duplication my tool detects and the merges it performs. Did you want it reverted because it was a bad detection or because it doesn't help readability of the Google developers.

"'$class_name$', (_service.Service,), dict(\n",
"class_name", descriptor.name());

void Generator::PrintDescriptorKeyAndModuleName(const ServiceDescriptor& descriptor) const {
printer_->Indent();

This comment has been minimized.

Copy link
@xfxyjwf

xfxyjwf Dec 11, 2014

Contributor

Move this "printer_->Indent()" to PrintServiceClass() and PrintServiceStub().

@@ -597,22 +594,21 @@ void Generator::PrintServiceClass(const ServiceDescriptor& descriptor) const {
printer_->Outdent();

This comment has been minimized.

Copy link
@xfxyjwf

xfxyjwf Dec 11, 2014

Contributor

Move "printer_->Print("))\n\n"); printer_->Outdent();" to PrintServiceClass() and PrintServiceStub().

A print function should print the same number of "("s and closing ")"s (if otherwise its calling function will have no idea whether it should produce extra "("s or ")"s). As this PrintDescriptorKeyAndModuleName() prints no "(", all of ")"s should be moved outside of this function.

@@ -511,4 +511,4 @@ bool IsStringOrMessage(const FieldDescriptor* field) {
} // namespace cpp
} // namespace compiler
} // namespace protobuf
} // namespace google
} // namespace google

This comment has been minimized.

Copy link
@xfxyjwf

xfxyjwf Dec 11, 2014

Contributor

Revert this change.

@xiaofeng-gg

This comment has been minimized.

Copy link
Contributor

xiaofeng-gg commented Dec 12, 2014

On Wed, Dec 10, 2014 at 10:48 PM, krishnanm86 notifications@github.com
wrote:

Implemented the feedback. Hope we can land it now.

Anyway could I ask you feedback on the merge which you wanted to revert. I
am collecting data on the duplication my tool detects and the merges it
performs. Did you want it reverted because it was a bad detection or
because it doesn't help readability of the Google developers.

Because it doesn't help the code's readability.


Reply to this email directly or view it on GitHub
#128 (comment).

krinara86 added 3 commits Dec 12, 2014
…printserviceclass in python generator
@krishnanm86

This comment has been minimized.

Copy link
Contributor Author

krishnanm86 commented Dec 12, 2014

Done.

xfxyjwf added a commit that referenced this pull request Dec 12, 2014
Source code refactoring. Extracted common functionality in cpp_helpers.c...
@xfxyjwf xfxyjwf merged commit f473bb9 into protocolbuffers:master Dec 12, 2014
1 check passed
1 check passed
cla/google All necessary CLAs are signed
TeBoring added a commit to TeBoring/protobuf that referenced this pull request Jan 19, 2019
* Fix json ignore unknown

Previously, there were several problems with ignoring unknown in json.
1) After finding a field is unknown, the parser's state is not changed. Thus, there is no way to distinguish whether the parser is dealing with an unknown field or it's just a top level message.
2) Several method didn't respect unknown field, e.g., start_object, end_bool, start_array.

* Update json parser size

* Update json parser size
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.