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

_get_str conflict for tuple with str field. #23

Closed
P-p-H-d opened this issue Aug 21, 2018 · 8 comments
Closed

_get_str conflict for tuple with str field. #23

P-p-H-d opened this issue Aug 21, 2018 · 8 comments
Assignees
Labels

Comments

@P-p-H-d
Copy link
Owner

P-p-H-d commented Aug 21, 2018

A tuple definition will generate the following methods:

  • _get_str: to transform the tuple into a string_t
  • _get_##field: to get the value of the field. This is done for each field.

If one field of the tuple is 'str', there is a conflict of method...

The issue is in the API definition and only an API breakage may solve it. I see multiple solution:

  • rename all _get_str methods into _convert_str for all types. I don't like this solution too much impact.
  • rename the _get_##field into something else. in fact, current function return a pointer to constant type; which means the _get_ function should have been called _cget_.

So I propose to change the functions _get_##field to _cget_##field

@mario-tux
Copy link

On first glance I would say that the second option would be acceptable and less intrusive but I see the same problem with variants: there the return value type is not constant and you can't exploit such coincidence.
Looking at the GMP naming you should even use get_str/set_str but this would be a further complication...

@P-p-H-d
Copy link
Owner Author

P-p-H-d commented Aug 21, 2018

True...

I propose then to have for both tuple & variant:

  • T * _get_at_##field
  • T const * _cget_at_##field

It may be good to have also:

  • T * _get(tuple, enum field)
  • T const * _cget(tuple, enum field)
    with the enum field defining which version of the field we want.
    (this will give a good symmetry with an array / dict _get & _cget).

@mario-tux
Copy link

This proposal looks reasonable with a safer naming.

I'm still looking at the strange naming of the _set_str/_parse_str methods. I understand that the _parse_str is one of the methods of an OPLIST and that it gets a third parameter to make possible the step-by-step parsing. On the other side I look at the string_set_str specific method and to the mpz_set_str one of GMP. I'm wondering if would be more congruent to keep the _parse_str methods with three params but to make standard a _set_str method for all the containers that get only two params and, maybe using _parse_str, parse the full string in order to reconstruct the container content. Does it make sense?

@P-p-H-d
Copy link
Owner Author

P-p-H-d commented Aug 21, 2018

Thanks.

The parse_str interface is based on the strtol interface, whereas set_str is based on atol. In fact, I think a method like parse_str is missing in GMP (MPFR has added mpfr_strtofr). The method _set_str is too limited for its designed usage. Moreover, there is the strict equivalence:
_set\_str(Object, str) == _parse_str(Object, str, NULL)
(_parse_str accepts NULL as final argument).

This different interface is why I chose a different naming for this function. Moreover, it reflects more what it does since for non-trivial types, it performs a parsing of the string. _set_str is suitable only for simple and controlled cases.

Note that string_set_str != string_parse_str: string_set_str sets the string to the exact array of char string_parse_str parses the string that shall be in the form "THIS \"IS\" A STRING" into THIS "IS" A STRING (this is needed for proper serialization of objects within other objects).

Therefore, I don't see the needs for adding _set_str.

@mario-tux
Copy link

mario-tux commented Aug 21, 2018

Ok, I can see the difference. Now it is more clear.

I've just tried the latest version in master and maybe something went wrong. Now my main code raises a warning like:

 warning: passing 'const symbol_list_t' (aka 'struct symbol_list_s const[1]') to parameter of type 'struct symbol_list_s *' discards qualifiers [-Wincompatible-pointer-types-discards-qualifiers]
TUPLE_DEF2(record, (name, string_t), (key, symbol_t),

on a tuple of mine:

TUPLE_DEF2(record, (name, string_t), (key, symbol_t), (fields, symbol_list_t))

If you tests say that all is ok, I can try to isolate the exact case.

@P-p-H-d
Copy link
Owner Author

P-p-H-d commented Aug 21, 2018

I cannot reproduce the issue. Could you open a bug report (with the function that raises the warning)?

@P-p-H-d
Copy link
Owner Author

P-p-H-d commented Aug 21, 2018

Methods have been renamed into _get_at and new methods _cget_at have been added.

@P-p-H-d
Copy link
Owner Author

P-p-H-d commented Aug 23, 2018

I don't see any way to implement the T * _get(tuple, enum field) method, as this method should be able to return different type depending on the fields.

@P-p-H-d P-p-H-d closed this as completed Aug 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants