Re: [PATCH] libtracefs: Prefer using const pointer arguments

["Yordan Karadzhov (VMware)" <y.karadz@gmail.com>]

On 22.03.21 г. 17:47, Steven Rostedt wrote:
> On Mon, 22 Mar 2021 16:29:12 +0200
> "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:
> 
>> All functions should receive const pointer arguments, except in the
>> cases when the argument object itself needs to be modified.
>>
>> Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
>> ---
>>   include/tracefs.h      | 15 ++++++++-------
>>   src/tracefs-instance.c | 17 +++++++++--------
>>   2 files changed, 17 insertions(+), 15 deletions(-)
>>
>> diff --git a/include/tracefs.h b/include/tracefs.h
>> index f3eec62..e54f791 100644
>> --- a/include/tracefs.h
>> +++ b/include/tracefs.h
>> @@ -25,12 +25,13 @@ struct tracefs_instance *tracefs_instance_create(const char *name);
>>   struct tracefs_instance *tracefs_instance_alloc(const char *tracing_dir,
>>   						const char *name);
>>   int tracefs_instance_destroy(struct tracefs_instance *instance);
>> -bool tracefs_instance_is_new(struct tracefs_instance *instance);
>> -const char *tracefs_instance_get_name(struct tracefs_instance *instance);
>> -const char *tracefs_instance_get_trace_dir(struct tracefs_instance *instance);
>> +bool tracefs_instance_is_new(const struct tracefs_instance *instance);
>> +const char *tracefs_instance_get_name(const struct tracefs_instance *instance);
>> +const char *tracefs_instance_get_trace_dir(const struct tracefs_instance *instance);
>>   char *
>> -tracefs_instance_get_file(struct tracefs_instance *instance, const char *file);
>> -char *tracefs_instance_get_dir(struct tracefs_instance *instance);
>> +tracefs_instance_get_file(const struct tracefs_instance *instance,
>> +			  const char *file);
>> +char *tracefs_instance_get_dir(const struct tracefs_instance *instance);
>>   int tracefs_instance_file_write(struct tracefs_instance *instance,
>>   				const char *file, const char *str);
>>   char *tracefs_instance_file_read(struct tracefs_instance *instance,
>> @@ -42,8 +43,8 @@ int tracefs_instance_file_open(struct tracefs_instance *instance,
>>   int tracefs_instances_walk(int (*callback)(const char *, void *), void *context);
>>   
>>   bool tracefs_instance_exists(const char *name);
>> -bool tracefs_file_exists(struct tracefs_instance *instance, char *name);
>> -bool tracefs_dir_exists(struct tracefs_instance *instance, char *name);
>> +bool tracefs_file_exists(struct tracefs_instance *instance, const char *name);
>> +bool tracefs_dir_exists(struct tracefs_instance *instance, const char *name);
>>   
>>   int tracefs_trace_is_on(struct tracefs_instance *instance);
>>   int tracefs_trace_on(struct tracefs_instance *instance);
>>
> 
> My fear about doing the above is that when dealing with complex structures
> like tracefs_instance, which is opaque (private) to the caller that it now
> prevents us from ever modifying the instance in one of those APIs. It may
> not modify it now, but that does not mean it wont be modified in the
> future. We could add ref counters, or something else.
> 

Hi Steven,

I understand your preference to keep the flexibility of those API, 
however generally spiking those are "getter" functions and they are not 
supposed to modify the encapsulated object. If we allow the "getters" of 
the API to modify the encapsulated object (tracefs_instancein this case) 
this will be a bit of hacking stile of programing. At least for me, 
having a firm and intuitive design of the API is more valuable than the 
inconvenience that this reduced flexibility can potentially bring.

Thanks!
Yordan


> What's the reason to make them constant? The caller does not have access to
> the internals of what those are pointing to. It's just an abstract object.
> Why would they care if its const or not?
> 
> -- Steve
>