Getting files C++ compilable #12499

yashykt · 2017-09-12T00:32:58Z

Getting files C++ compilable - Adding pointer conversions, changing variable and type names.

yashykt · 2017-09-12T17:38:17Z

Jenkins: test this please

vjpai

In general, check if the things that you are typecasting have already been typecast the desired way.

vjpai · 2017-09-12T18:53:59Z

src/core/lib/iomgr/ev_epoll1_linux.c

@@ -130,9 +130,9 @@ static void fd_global_shutdown(void);
 * Pollset Declarations
 */

-typedef enum { UNKICKED, KICKED, DESIGNATED_POLLER } kick_state;
+typedef enum { UNKICKED, KICKED, DESIGNATED_POLLER } kick_state_t;


Just curious, what was the motivation for going from kick_state to kick_state_t ? Is this a style guide rule?

And likewise with the other places where _t is getting appended to struct or enum names.

C++ does not allow same type and variable names. There were cases such as :
kick_state kick_state;
Adding a _t seemed like the easiest way to solve them.

vjpai · 2017-09-12T18:55:06Z

src/core/lib/iomgr/polling_entity.h

@@ -22,6 +22,8 @@
 #include "src/core/lib/iomgr/pollset.h"
 #include "src/core/lib/iomgr/pollset_set.h"

+typedef enum pops_tag { POPS_NONE, POPS_POLLSET, POPS_POLLSET_SET } pops_tag;


Hmm, this abbreviation isn't obvious to me. I think stylistically, non-obvious abbrevations shouldn't be used.

I've just brought it outside the struct, since C++ does not allow using enum which are inside a struct.

Ah, ok, this is uglier then. Since this is in a header and lives outside of a struct, it is polluting the global namespace. (FWIW, this was probably true even before since C doesn't have the concept of local names afaik, but we didn't particularly notice it for some reason.) Anything in the global namespace needs to be prefixed grpc_ . But I meant that the enum names are ugly. And even these enums should be prefixed GRPC_ .

The grpc_ prefix is for functions as per https://github.com/grpc/grpc/blob/master/doc/c-style-guide.md. But it might make sense ti add I guess we can substitute it with
typedef enum pollset_tag {POLLS_NONE, POLLS_POLLSET, POLLS_POLLSET_SET} pollset_tag;
or
typedef enum grpc_pollset_tag {GRPC_POLLS_NONE, GRPC_POLLS_POLLSET, GRPC_POLLS_POLLSET_SET} grpc_pollset_tag;

Then this is a defect in our C style guide. In practice, all the structs/enums declared in header files have the typename prefixed with grpc_ and all the enum names are also prefixed with GRPC_ . You should go ahead and fix this as typedef enum grpc_pollset_tag .... etc (your 2nd option above)

I'll fix the style guide.

Thanks Vijay! Done

I have sent in PR #12519 to fix the style guide (as well as a few places where we weren't actually following the rules)

vjpai · 2017-09-12T18:56:46Z

src/core/lib/surface/call.c

@@ -549,8 +549,8 @@ static void destroy_call(grpc_exec_ctx *exec_ctx, void *call,
    GRPC_CQ_INTERNAL_UNREF(exec_ctx, c->cq, "bind");
  }

-  get_final_status(call, set_status_value_directly, &c->final_info.final_status,
-                   NULL);
+  get_final_status((grpc_call *)call, set_status_value_directly,


Wait, this has already been typecasted to a call as c above (in line 527)

Yes. Good eye!

vjpai · 2017-09-12T18:57:34Z

src/core/lib/surface/call.c

@@ -1474,7 +1475,7 @@ static void receiving_stream_ready(grpc_exec_ctx *exec_ctx, void *bctlp,
   * acq_load is in receiving_initial_metadata_ready() */
  if (error != GRPC_ERROR_NONE || call->receiving_stream == NULL ||
      !gpr_atm_rel_cas(&call->recv_state, RECV_NONE, (gpr_atm)bctlp)) {
-    process_data_after_md(exec_ctx, bctlp);
+    process_data_after_md(exec_ctx, (batch_control *)bctlp);


bctl , right?

vjpai

Please address the comments in my previous review...

grpc-kokoro · 2017-09-12T20:31:46Z

[trickle] No significant performance differences

grpc-kokoro · 2017-09-12T20:32:08Z

[microbenchmarks] No significant performance differences

vjpai · 2017-09-12T20:32:40Z

BTW, what are the strange commits at the top of this PR? There seem to be commits from Mark and others that are unrelated. Looks to me like your branch base is questionable; can you rebase this to master?

…o avoid C++ compilation issues

yashykt · 2017-09-12T22:03:10Z

I was trying out git rebase -i. So it intermingled some of the merges. Fixed it.

vjpai

LGTM

grpc-kokoro · 2017-09-12T23:20:24Z

[trickle] No significant performance differences

grpc-kokoro · 2017-09-12T23:59:22Z

[microbenchmarks] No significant performance differences

yashykt · 2017-09-13T05:37:10Z

Known issues : #11737, (also the fixed INT_MAX issue in flow_control.c)

ctiller · 2017-09-13T13:27:17Z

_t names are reserved for the language and system libraries afaik... Can we change this to use something else?

yashykt · 2017-09-13T15:43:33Z

Can change it to explicitly use _type or _enum or something else. What do you suggest?

ctiller · 2017-09-13T16:15:16Z

_type or _enum would be fine: my expectation is that we'll rename these again once we start c++-ifying the codebase, but until then we should stay within spec and the existing style guide.

…

On Wed, Sep 13, 2017 at 8:44 AM Yash Tibrewal ***@***.***> wrote: Can change it to explicitly use _type or _enum or something else. What do you suggest? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#12499 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJpudcX8fz_qqvXt-oO0q9cPdftgT2xVks5sh_hMgaJpZM4PT8Wd> .

yashykt · 2017-09-13T17:22:34Z

Created PR #12545 to change _t to _type

googlebot added the cla: yes label Sep 12, 2017

nathanielmanistaatgoogle added the area/core label Sep 12, 2017

vjpai reviewed Sep 12, 2017

View reviewed changes

vjpai suggested changes Sep 12, 2017

View reviewed changes

yashykt force-pushed the ctocc3 branch from c1b5409 to 4cb535a Compare September 12, 2017 19:18

yashykt added 3 commits September 12, 2017 14:59

Adding pointer conversions. Renaming a few variables and type names t…

52778c4

…o avoid C++ compilation issues

Using already type casted variables

090aca5

Renaming pops_tag enum

97dab51

yashykt force-pushed the ctocc3 branch from c06746a to 97dab51 Compare September 12, 2017 22:01

vjpai approved these changes Sep 12, 2017

View reviewed changes

yashykt merged commit 22e89a8 into grpc:master Sep 13, 2017

yashykt mentioned this pull request Sep 13, 2017

Removing _t type suffix and changing field names instead #12545

Merged

yashykt deleted the ctocc3 branch March 14, 2018 22:12

lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Getting files C++ compilable #12499

Getting files C++ compilable #12499

yashykt commented Sep 12, 2017

yashykt commented Sep 12, 2017

vjpai left a comment

vjpai Sep 12, 2017

vjpai Sep 12, 2017

yashykt Sep 12, 2017

vjpai Sep 12, 2017

vjpai Sep 12, 2017

yashykt Sep 12, 2017

vjpai Sep 12, 2017 •

edited

Loading

yashykt Sep 12, 2017

vjpai Sep 12, 2017

vjpai Sep 12, 2017

yashykt Sep 12, 2017

vjpai Sep 12, 2017

vjpai Sep 12, 2017

yashykt Sep 12, 2017

vjpai Sep 12, 2017

yashykt Sep 12, 2017

vjpai left a comment

grpc-kokoro commented Sep 12, 2017

grpc-kokoro commented Sep 12, 2017

vjpai commented Sep 12, 2017

yashykt commented Sep 12, 2017

vjpai left a comment

grpc-kokoro commented Sep 12, 2017

grpc-kokoro commented Sep 12, 2017

yashykt commented Sep 13, 2017

ctiller commented Sep 13, 2017

yashykt commented Sep 13, 2017

ctiller commented Sep 13, 2017 via email

yashykt commented Sep 13, 2017

Getting files C++ compilable #12499

Getting files C++ compilable #12499

Conversation

yashykt commented Sep 12, 2017

yashykt commented Sep 12, 2017

vjpai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vjpai Sep 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vjpai left a comment

Choose a reason for hiding this comment

grpc-kokoro commented Sep 12, 2017

grpc-kokoro commented Sep 12, 2017

vjpai commented Sep 12, 2017

yashykt commented Sep 12, 2017

vjpai left a comment

Choose a reason for hiding this comment

grpc-kokoro commented Sep 12, 2017

grpc-kokoro commented Sep 12, 2017

yashykt commented Sep 13, 2017

ctiller commented Sep 13, 2017

yashykt commented Sep 13, 2017

ctiller commented Sep 13, 2017 via email

yashykt commented Sep 13, 2017

vjpai Sep 12, 2017 •

edited

Loading