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 into merged methods #126

Closed

Conversation

krishnanm86
Copy link

Source code refactoring. Extracted common functionality into merged methods

@googlebot
Copy link

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.

@googlebot
Copy link

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Dec 9, 2014
@xfxyjwf
Copy link
Contributor

xfxyjwf commented Dec 9, 2014

I think this refactoring doesn't improve the code's readability and also it does not conform to Google C++ Style guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html

@krishnanm86
Copy link
Author

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_");
Copy link
Contributor

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.

@xfxyjwf xfxyjwf closed this Dec 10, 2014
@xfxyjwf
Copy link
Contributor

xfxyjwf commented Dec 10, 2014

@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.

@krishnanm86
Copy link
Author

#128

Created the pull request. Hope it reached you and hope it satisfies the
google code requirements.

On Wed, Dec 10, 2014 at 8:21 PM, Feng Xiao notifications@github.com wrote:

@krishnanm86 https://github.com/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.


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


I dare do all that may become a man; Who dares do more, is none - Macbeth,
twelfh night!
Regards
Krishna

TeBoring added a commit to TeBoring/protobuf that referenced this pull request Jan 19, 2019
…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}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants