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

Change functions to accept const ptrs to allocators #89

Merged
merged 2 commits into from
Feb 26, 2018
Merged

Conversation

dhood
Copy link
Member

@dhood dhood commented Feb 5, 2018

In ros2/rcl#212, I wanted to pass a const rcl_allocator_t * to repl_str, but got warnings that the parameter wasn't const.

@dirk-thomas and I spoke about that and were concerned that the functions that currently take const ptrs actually should not, because the state of the allocator can change when the allocator's used. We theorised that a warning about modifying a const variable (when allocate is called in a function that takes const rcutils_allocator_t *) wasn't being generated because of the type erasure in the signature.

@wjwwood pointed out that the state is actually stored as a pointer, so it's fair to use const since these functions never reassign the pointer. That's also why it's fair to pass an allocator by value in other places: it will still impact the state of the original allocator (another thing @dirk-thomas and I were concerned about).

Therefore this PR just changes the remaining functions to take a const rcutils_allocator_t * since they don't modify the struct, so that it can be called from places like rcl_node_init which is using a const pointer.

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@dhood dhood added the in review Waiting for review (Kanban column) label Feb 5, 2018
@dhood dhood self-assigned this Feb 5, 2018
@dirk-thomas
Copy link
Member

Since conceptionally the state of the allocator is changing when it is being used (think about it that it keeps track of the allocated memory) I don't think that it should be const in these cases.

@dhood
Copy link
Member Author

dhood commented Feb 5, 2018

I know what you mean, but the allocator struct itself only has a pointer to the state though, which is not modified

@dirk-thomas
Copy link
Member

I understand that it technically works with const. I still don't think it should be const. E.g. if we would ever change the allocator type to contain the state information directly the API would fail to work.

@wjwwood
Copy link
Member

wjwwood commented Feb 5, 2018

Since conceptionally [sic] the state of the allocator is changing when it is being used (think about it that it keeps track of the allocated memory) I don't think that it should be const in these cases.

I disagree.

The const only technically applies to the pointer values in the allocator struct. Since the state is opaque, you cannot know if it is being modified or not and in fact does not matter.

The contract on a function that takes a const pointer to an allocator is that the allocator struct itself is not changed, not anything and everything it might reference directly or indirectly.

Even if it keeps track of the allocated memory, I don't think making the allocator struct const implies that cannot result in changes to the state.

I understand that it technically works with const. I still don't think it should be const. E.g. if we would ever change the allocator type to contain the state information directly the API would fail to work.

There is no plan (or foreseeable reason) to do that.

What using const here does is prevent (without a const_cast equivalent) the function which takes an allocator pointer from changing the state pointer or any of the allocate/deallocate like function pointers in the struct.

I might have a function which replaces the malloc function on an allocator struct with a new one, that function would need to have a non-const pointer. That's an example where this use of const differentiates two valid use cases, neither of which have to do with the const-ness of the state value itself.

@dirk-thomas
Copy link
Member

There is no plan (or foreseeable reason) to do that.

Still the fact remains that the API design (using const) will prohibit changing the allocator state in the future which in my opinion is very bad coupling of two unrelated things.

@wjwwood
Copy link
Member

wjwwood commented Feb 5, 2018

Still the fact remains that the API design (using const) will prohibit changing the allocator state in the future which in my opinion is very bad coupling of two unrelated things.

That's not true, you can always store arbitrary (and mutable) state in the state object. We could simply change the void * of state to be a struct like:

struct rcutils_allocator_state_t {
  size_t malloc_call_count;
  size_t free_call_count;
  void * implementation_specific_data;
}

And I will reiterate that there is no reasonably foreseeable requirement or plan to do the above. We can't design our API to allow for any conceivably possible future change.

If C were more expressive or this were C++, then I'd agree with you can we could use non-const pointers to allocator's because then we would just make the function points and the state pointer const in the class (setting them only on construction instead). But that's not the case here.

@wjwwood
Copy link
Member

wjwwood commented Feb 5, 2018

I suppose we actually could make the members of the allocator type const. I was wrong that C doesn't support that.

We'd just have to cast away the const when creating the allocators in the first place or use an initializer list (haven't tried the latter).

If that works, then I'd be convinced to leave the pointers to the allocators non-cost is the right thing to do.

@wjwwood
Copy link
Member

wjwwood commented Feb 5, 2018

The initializer lists do work. @dhood and @dirk-thomas is that an acceptable compromise? Using const pointers (not const values) for the function pointers and state in the allocator struct and then taking non-const pointers to allocators in all function parameters (inverse of this pr)?

@wjwwood
Copy link
Member

wjwwood commented Feb 5, 2018

The initializer lists do work.

What I mean actually is the initializer list like syntax, I guess it's actually C-specific struct initialization syntax.

@Karsten1987
Copy link
Contributor

What about passing it like rcutils_allocator_t * const allocator? This still allows you to change the members of the struct, however prevents the deallocation/reallocation of the actual allocator struct.

@dirk-thomas
Copy link
Member

is that an acceptable compromise? Using const pointers (not const values) for the function pointers and state in the allocator struct and then taking non-const pointers to allocators in all function parameters (inverse of this pr)?

Depending on how the const is being used in the struct that sounds better to me. Can you post a snippet with the proposed code (or update the PR)?

@wjwwood
Copy link
Member

wjwwood commented Feb 6, 2018

What about passing it like rcutils_allocator_t * const allocator? This still allows you to change the members of the struct, however prevents the deallocation/reallocation of the actual allocator struct.

But this doesn't actually matter, because with or without the const in your signature above, changing the pointer within the function doesn't change it outside. E.g.:

typedef struct {
  int i;
  int j;
} rcutils_allocator_t;

void func1(rcutils_allocator_t * const allocator) {
  (void) allocator;
  // allocator = (rcutils_allocator_t *)0x02;  // compiler error
}

void func2(rcutils_allocator_t * allocator) {
  allocator = (rcutils_allocator_t *)0x03;
}

int main(void) {
  rcutils_allocator_t * allocator = (rcutils_allocator_t *)0x01;
  func1(allocator);
  // allocator == 0x01 still
  func2(allocator);
  // allocator == 0x01 even still
}

Depending on how the const is being used in the struct that sounds better to me. Can you post a snippet with the proposed code (or update the PR)?

I'll put together a pull request or iterate with @dhood on it as an option as soon as I have time.

@mikaelarguedas mikaelarguedas added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels Feb 6, 2018
@dhood
Copy link
Member Author

dhood commented Feb 15, 2018

I've been looking at swapping the struct to store e.g. void * const state;

There are a number of places where we update allocators stored by value, which with that change will give compiler errors e.g.

__rcutils_error_state->allocator = allocator;

@wjwwood confirmed that we don't want to restrict users from copying allocators; we can swap some instances to store pointers, but there are times when we store by value so they're not tied to the lifetime of the allocator passed in.

We concluded that we should not change the struct, but just remove const from the functions and from places like this. I'll work on that.

@dhood
Copy link
Member Author

dhood commented Feb 20, 2018

Removing const from all of the allocator references will also mean removing const from signatures that take in options structs e.g. rcl_node_init. This seemed too tightly coupled to the contents of the options struct (having an allocator), so we discussed the direction of this PR again as a team.

The conclusion upon reflection is that we are not opposed to the notion of const being used in signatures in the form of function(const rcutils_allocator_t * allocator); The intent is to communicate to the developer that the function will not modify the fields of the struct, not that "the allocator" as a concept will remain unchanged. The allocator state can indeed change inside functions with those signatures; the state is a conceptual exception in a similar way to a member marked as mutable in a c++ class.

I'll add documentation to this effect but otherwise we're satisfied with the original approach of this PR.

@dhood
Copy link
Member Author

dhood commented Feb 22, 2018

I added documentation of the mutable nature of the state even in const-qualified allocators in d36a4d2; open to input on the wording

@dhood dhood added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Feb 22, 2018
@dhood dhood requested a review from wjwwood February 23, 2018 19:23
@dhood dhood merged commit c72d2cf into master Feb 26, 2018
@dhood dhood removed the in review Waiting for review (Kanban column) label Feb 26, 2018
@dhood dhood deleted the const_alloc branch February 26, 2018 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants