[FFmpeg-devel] [PATCH 1/2] lavu/dict: Add new flag to allow multiple equal keys.

[Hendrik Leppkes]

On Fri, Mar 25, 2016 at 6:26 PM, Thilo Borgmann <thilo.borgmann at mail.de> wrote:
> Am 25.03.16 um 17:56 schrieb Hendrik Leppkes:
>> On Fri, Mar 25, 2016 at 5:48 PM, Thilo Borgmann <thilo.borgmann at mail.de> wrote:
>>> Am 22.03.16 um 12:20 schrieb Thilo Borgmann:
>>>> Am 22.03.16 um 11:45 schrieb wm4:
>>>>> On Sun, 13 Mar 2016 21:00:23 +0100
>>>>> Thilo Borgmann <thilo.borgmann at mail.de> wrote:
>>>>>
>>>>>> Am 13.03.16 um 15:08 schrieb wm4:
>>>>>>> On Sat, 12 Mar 2016 15:13:21 +0100
>>>>>>> Thilo Borgmann <thilo.borgmann at mail.de> wrote:
>>>>>>>
>>>>>>>> From a1d9ce388c69eabb256e6b351c2acd36d7f4076e Mon Sep 17 00:00:00 2001
>>>>>>>> From: Thilo Borgmann <thilo.borgmann at mail.de>
>>>>>>>> Date: Sat, 12 Mar 2016 14:52:17 +0100
>>>>>>>> Subject: [PATCH 1/2] lavu/dict: Add new flag to allow multiple equal keys.
>>>>>>>>
>>>>>>>> ---
>>>>>>>>  libavutil/dict.c | 5 ++++-
>>>>>>>>  libavutil/dict.h | 5 +++--
>>>>>>>>  2 files changed, 7 insertions(+), 3 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/libavutil/dict.c b/libavutil/dict.c
>>>>>>>> index 8bb65a1..70c0184 100644
>>>>>>>> --- a/libavutil/dict.c
>>>>>>>> +++ b/libavutil/dict.c
>>>>>>>> @@ -70,9 +70,12 @@ int av_dict_set(AVDictionary **pm, const char *key, const char *value,
>>>>>>>>                  int flags)
>>>>>>>>  {
>>>>>>>>      AVDictionary *m = *pm;
>>>>>>>> -    AVDictionaryEntry *tag = av_dict_get(m, key, NULL, flags);
>>>>>>>> +    AVDictionaryEntry *tag = NULL;
>>>>>>>>      char *oldval = NULL, *copy_key = NULL, *copy_value = NULL;
>>>>>>>>
>>>>>>>> +    if (!(flags & AV_DICT_MULTIKEY)) {
>>>>>>>> +        tag = av_dict_get(m, key, NULL, flags);
>>>>>>>> +    }
>>>>>>>>      if (flags & AV_DICT_DONT_STRDUP_KEY)
>>>>>>>>          copy_key = (void *)key;
>>>>>>>>      else
>>>>>>>> diff --git a/libavutil/dict.h b/libavutil/dict.h
>>>>>>>> index b0aa784..c589bcd 100644
>>>>>>>> --- a/libavutil/dict.h
>>>>>>>> +++ b/libavutil/dict.h
>>>>>>>> @@ -76,6 +76,7 @@
>>>>>>>>  #define AV_DICT_DONT_OVERWRITE 16   ///< Don't overwrite existing entries.
>>>>>>>>  #define AV_DICT_APPEND         32   /**< If the entry already exists, append to it.  Note that no
>>>>>>>>                                        delimiter is added, the strings are simply concatenated. */
>>>>>>>> +#define AV_DICT_MULTIKEY       64   /**< Allow to store several equal keys in the dictionary */
>>>>>>>>
>>>>>>>>  typedef struct AVDictionaryEntry {
>>>>>>>>      char *key;
>>>>>>>> @@ -118,8 +119,8 @@ int av_dict_count(const AVDictionary *m);
>>>>>>>>   *
>>>>>>>>   * @param pm pointer to a pointer to a dictionary struct. If *pm is NULL
>>>>>>>>   * a dictionary struct is allocated and put in *pm.
>>>>>>>> - * @param key entry key to add to *pm (will be av_strduped depending on flags)
>>>>>>>> - * @param value entry value to add to *pm (will be av_strduped depending on flags).
>>>>>>>> + * @param key entry key to add to *pm (will either be av_strduped or added as a new key depending on flags)
>>>>>>>> + * @param value entry value to add to *pm (will be av_strduped or added as a new key depending on flags).
>>>>>>>>   *        Passing a NULL value will cause an existing entry to be deleted.
>>>>>>>>   * @return >= 0 on success otherwise an error code <0
>>>>>>>>   */
>>>>>>>
>>>>>>> Changing the semantics of AVDictionary just like this seems rather
>>>>>>> questionable...
>>>>>>
>>>>>> It changes nothing for existing code, just adds a new feature. I don't
>>>>>> think it hurts anyone or anything...
>>>>>
>>>>> It only breaks basic assumptions about a basic data type...
>>>>
>>>> Although I don't share your thought about breaking a basic data type with that,
>>>> what would you suggest instead?
>>>
>>> Pushed for no further suggestions and nobody else objected.
>>>
>>
>> Just pushing without addressing concerns is not the way we usually try
>> to work here, just saying.
>
> I think I have quite a good idea about the usual way we try to handle
> things here and I think I've addressed his concerns.

If by addressing you mean disagreeing with the concern and doing nothing.

>
> He didn't like it which is of course ok. He did not continue discussing
> it nor did he proposed any alternative. He also did not pick up any of
> my thoughts. He also did not explicitly state that he thinks that it is
> not ok to apply it. He said it "seems rather questionable". Without
> further discussion (what I tried) and nobody else complaining about it,
> what do you think would be more appropriate than to wait for quite a
> long time until continuing?
>
> The usual way for him to prevent me pushing it would just have been to
> ask me to wait and I would have waited. Have you checked the dates of
> the replies and what I wrote before accusing me to just ignore concerns?
>

Timing makes no difference. Its the only review you got, so even if
you ignore that, you didn't even get a "OK" from anyone else, which
for generic API should be mandatory.
The least that would have been appropriate would be to ping the patch
asking for further comments, instead of just practically saying "I'm
done waiting and just pushing"

Not everyone has the time to answer within a day, so if someone
expressed a concern, the least one could do before pushing is asking
again, everything else feels rather disingenuous.

- Hendrik