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 into merged methods #126
Source code refactoring. Extracted common functionality into merged methods #126
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA) at https://cla.developers.google.com/. If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check the information on your CLA or see this help article on setting the email on your git commits. Once you've done that, please reply here to let us know. If you signed the CLA as a corporation, please let us know the company's name. |
CLAs look good, thanks! |
I think this refactoring doesn't improve the code's readability and also it does not conform to Google C++ Style guide: |
I am working on a research tool that identifies redundant code and performs merges . Could you tell me which one (or both) of the merges did you not like. And how I could improve it. I could work on a more meaningful commit . The print methods seem to have 2/3rds of the functionality repeated which I have merged. I could probably work on a more readable version of the merges if you could provide some comments. The Descriptor methods also had only a part of the string which seemed different. |
// Return the name of the AddDescriptors() function for a given file. | ||
string GlobalAddDescriptorsName(const string& filename) { | ||
return "protobuf_AddDesc_" + FilenameIdentifier(filename); | ||
return GlobalXName(filename,"protobuf_AddDesc_"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change doesn't improve the code. There are 3 symbols (a function name, a variable name and a string literal) both before and after the change.
@krishnanm86 , you can send me another pull request. Make sure the pull request is against master branch. This pull request is against branch/v3.0.0-alpha and as I deleted that branch this pull request was closed automatically. |
Created the pull request. Hope it reached you and hope it satisfies the On Wed, Dec 10, 2014 at 8:21 PM, Feng Xiao notifications@github.com wrote:
I dare do all that may become a man; Who dares do more, is none - Macbeth, |
…lbuffers#126) * Fix json encoding for wrappers, ListValue, Struct and Value. * Add well_known_type field in upb_msgdef to specify type of well known messages. * Remove comma at end of enum definition. * Group number wrappers * Fix comments * Refactoring to use is_wellknown_{msg/field}
Source code refactoring. Extracted common functionality into merged methods