Re: [ovs-dev] [PATCHv2 1/4] ovsdb-idl: Avoid class declaration.

[Ben Pfaff]

On Thu, Aug 10, 2017 at 02:31:43PM -0700, Joe Stringer wrote:
> On 10 August 2017 at 11:41, Ben Pfaff  wrote:
> > On Thu, Aug 10, 2017 at 01:01:32PM +0800, Gao Zhenyu wrote:
> >> Besides of that, I see many places consume the table class.
> >> Do you mind to make a macro helps to fetch the class?
> >> Like:
> >>
> >> #define OVSDB_GET_TABLE_CLASS(row) \
> >> ((row)->table->class)
> >
> > Ugh.
> 
> Just in case the context isn't obvious... ;-)
> 
> I think that in general, we try not to obscure what's going on in OVS
> code. This particular suggestion makes it less clear when reading the
> code exactly where the class comes from, because the pointer
> dereference is then hidden behind the macro. It also makes lines
> referencing the class longer, and some of these lines are already
> fairly long.

That's a much better way of explaining what I meant.  Thanks, Joe.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCHv2 1/4] ovsdb-idl: Avoid class declaration.

[Joe Stringer]

On 10 August 2017 at 11:41, Ben Pfaff  wrote:
> On Thu, Aug 10, 2017 at 01:01:32PM +0800, Gao Zhenyu wrote:
>> How about:
>> struct ovsdb_idl_table {
>> ...
>> const struct ovsdb_idl_table_class *table_class
>> 
>> }
>>
>> struct ovsdb_idl {
>> 
>>  const struct ovsdb_idl_class *idl_class;
>> 
>
> Why make it longer?
>
>> Besides of that, I see many places consume the table class.
>> Do you mind to make a macro helps to fetch the class?
>> Like:
>>
>> #define OVSDB_GET_TABLE_CLASS(row) \
>> ((row)->table->class)
>
> Ugh.

Just in case the context isn't obvious... ;-)

I think that in general, we try not to obscure what's going on in OVS
code. This particular suggestion makes it less clear when reading the
code exactly where the class comes from, because the pointer
dereference is then hidden behind the macro. It also makes lines
referencing the class longer, and some of these lines are already
fairly long.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCHv2 1/4] ovsdb-idl: Avoid class declaration.

[Ben Pfaff]

On Thu, Aug 10, 2017 at 01:01:32PM +0800, Gao Zhenyu wrote:
> How about:
> struct ovsdb_idl_table {
> ...
> const struct ovsdb_idl_table_class *table_class
> 
> }
> 
> struct ovsdb_idl {
> 
>  const struct ovsdb_idl_class *idl_class;
> 

Why make it longer?

> Besides of that, I see many places consume the table class.
> Do you mind to make a macro helps to fetch the class?
> Like:
> 
> #define OVSDB_GET_TABLE_CLASS(row) \
> ((row)->table->class)

Ugh.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCHv2 1/4] ovsdb-idl: Avoid class declaration.

[Gao Zhenyu]

How about:
struct ovsdb_idl_table {
...
const struct ovsdb_idl_table_class *table_class

}

struct ovsdb_idl {

 const struct ovsdb_idl_class *idl_class;


Besides of that, I see many places consume the table class.
Do you mind to make a macro helps to fetch the class?
Like:

#define OVSDB_GET_TABLE_CLASS(row) \
((row)->table->class)

Thanks
Zhenyu Gao

2017-08-10 6:27 GMT+08:00 Joe Stringer :

> In C++, 'class' is a keyword. If this is used as the name for a field,
> then C++ compilers can get confused about the context and fail to
> compile references to such fields. Rename the field to 'class_' to
> avoid this issue.
>
> Signed-off-by: Joe Stringer 
> ---
> v2: Rebase.
> ---
>  lib/db-ctl-base.c|   4 +-
>  lib/ovsdb-idl-provider.h |   2 +-
>  lib/ovsdb-idl.c  | 154 +++---
> -
>  3 files changed, 80 insertions(+), 80 deletions(-)
>
> diff --git a/lib/db-ctl-base.c b/lib/db-ctl-base.c
> index d597c6c2af6f..9fec6fa0d59e 100644
> --- a/lib/db-ctl-base.c
> +++ b/lib/db-ctl-base.c
> @@ -635,7 +635,7 @@ check_mutable(const struct ovsdb_idl_row *row,
>  {
>  if (!ovsdb_idl_is_mutable(row, column)) {
>  ctl_fatal("cannot modify read-only column %s in table %s",
> -  column->name, row->table->class->name);
> +  column->name, row->table->class_->name);
>  }
>  }
>
> @@ -1715,7 +1715,7 @@ cmd_show_find_table_by_row(const struct
> ovsdb_idl_row *row)
>  const struct cmd_show_table *show;
>
>  for (show = cmd_show_tables; show->table; show++) {
> -if (show->table == row->table->class) {
> +if (show->table == row->table->class_) {
>  return show;
>  }
>  }
> diff --git a/lib/ovsdb-idl-provider.h b/lib/ovsdb-idl-provider.h
> index 59fb24015904..a2eb8cac67d7 100644
> --- a/lib/ovsdb-idl-provider.h
> +++ b/lib/ovsdb-idl-provider.h
> @@ -104,7 +104,7 @@ struct ovsdb_idl_table_class {
>  };
>
>  struct ovsdb_idl_table {
> -const struct ovsdb_idl_table_class *class;
> +const struct ovsdb_idl_table_class *class_;
>  unsigned char *modes;/* OVSDB_IDL_* bitmasks, indexed by column.
> */
>  bool need_table; /* Monitor table even if no columns are
> selected
>* for replication. */
> diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
> index e916e940b652..227aa5fbfcb2 100644
> --- a/lib/ovsdb-idl.c
> +++ b/lib/ovsdb-idl.c
> @@ -89,11 +89,11 @@ enum ovsdb_idl_state {
>  };
>
>  struct ovsdb_idl {
> -const struct ovsdb_idl_class *class;
> +const struct ovsdb_idl_class *class_;
>  struct jsonrpc_session *session;
>  struct uuid uuid;
>  struct shash table_by_name; /* Contains "struct ovsdb_idl_table *"s.*/
> -struct ovsdb_idl_table *tables; /* Array of ->class->n_tables
> elements. */
> +struct ovsdb_idl_table *tables; /* Array of ->class_->n_tables
> elements. */
>  unsigned int change_seqno;
>  bool verify_write_only;
>
> @@ -270,7 +270,7 @@ ovsdb_idl_create(const char *remote, const struct
> ovsdb_idl_class *class,
>  : 0);
>
>  idl = xzalloc(sizeof *idl);
> -idl->class = class;
> +idl->class_ = class;
>  idl->session = jsonrpc_session_open(remote, retry);
>  shash_init(>table_by_name);
>  idl->tables = xmalloc(class->n_tables * sizeof *idl->tables);
> @@ -280,7 +280,7 @@ ovsdb_idl_create(const char *remote, const struct
> ovsdb_idl_class *class,
>  size_t j;
>
>  shash_add_assert(>table_by_name, tc->name, table);
> -table->class = tc;
> +table->class_ = tc;
>  table->modes = xmalloc(tc->n_columns);
>  memset(table->modes, default_mode, tc->n_columns);
>  table->need_table = false;
> @@ -338,7 +338,7 @@ ovsdb_idl_destroy(struct ovsdb_idl *idl)
>  ovsdb_idl_clear(idl);
>  jsonrpc_session_close(idl->session);
>
> -for (i = 0; i < idl->class->n_tables; i++) {
> +for (i = 0; i < idl->class_->n_tables; i++) {
>  struct ovsdb_idl_table *table = >tables[i];
>  ovsdb_idl_condition_destroy(>condition);
>  ovsdb_idl_destroy_indexes(table);
> @@ -363,7 +363,7 @@ ovsdb_idl_clear(struct ovsdb_idl *idl)
>  bool changed = false;
>  size_t i;
>
> -for (i = 0; i < idl->class->n_tables; i++) {
> +for (i = 0; i < idl->class_->n_tables; i++) {
>  struct ovsdb_idl_table *table = >tables[i];
>  struct ovsdb_idl_row *row, *next_row;
>
> @@ -768,9 +768,9 @@ ovsdb_idl_check_consistency(const struct ovsdb_idl
> *idl)
>  struct uuid *dsts = NULL;
>  size_t allocated_dsts = 0;
>
> -for (size_t i = 0; i < idl->class->n_tables; i++) {
> +for (size_t i = 0; i < idl->class_->n_tables; i++) {
>  const struct ovsdb_idl_table *table = >tables[i];
> -const struct ovsdb_idl_table_class *class = table->class;
> +const struct