This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 1/4] introduce struct strlen_data_t into gimple-fold


On 12/10/18 2:00 PM, Martin Sebor wrote:
> Jeff, is there something you are expecting me to change in
> response to this or have you just not gotten around to reviewing
> the rest?
Your last comment led me to believe you had another iteration on the
first patch to do before we moved forward.


> 
> Martin
> 
> On 11/28/18 9:26 PM, Jeff Law wrote:
>> On 11/25/18 5:05 PM, Martin Sebor wrote:
>>>
>>>> If so, then I think we need
>>>> to look for a better name than MAXSIZE and MAXLEN.
>>>
>>> I find these names quite fitting and I'm not sure what might work
>>> better.  I renamed MAXSIZE to MAXBOUND but nothing comes to mind
>>> as a replacement for MAXLEN.  Please suggest something you think
>>> is better.
>>>
>>>>
>>>>> +  /* When non-null, NONSTR refers to the declaration known to store
>>>>> +     an unterminated constant character array, as in:
>>>>> +     const char s[] = { 'a', 'b', 'c' };
>>>>> +     It is used to diagnose uses of such arrays in functions such as
>>>>> +     strlen() that expect a nul-terminated string as an argument.  */
>>>>> +  tree nonstr;
>>>> So rather than NONSTR, DECL may make more sense -- if for no other
>>>> reason than you don't have to think in terms of "not a string".
>>>
>>> Done, but I think DECL is a poor choice for the reasons below.
>>>
>>> The field is only set when the thing the object refers to is
>>> a character array that is not a string.  It identifies the first
>>> array the expression refers to that's not a terminated string
>>> (there could be multiple).  I can't think of anything else one
>>> might want to think of it as than "a declaration of an array
>>> that is not a string."
>>>
>>> As a name, DECL is generic and used all over the place for any
>>> sort of a declaration so it's not a good choice also for that
>>> reason.  It's only marginally more descriptive that the pervasive
>>> NODE or T, but just as useless to grep for (which I have been
>>> relying on when working with this patch).
>>>
>>> I have been using the name NONSTR in all contexts where
>>> I introduced the unterminated array handling, so renaming
>>> the member to DECL makes this scheme inconsistent
>> NONSTR requires you to think in the negative and it sounds more like a
>> boolean property to me, but it's actually carrying more information than
>> just a boolean.
>>
>> I'm certainly not wed to DECL.  If you've got a better name, please
>> suggest one.
>>
>>
>>>
>>>>> +  /* ELTSIZE is set to 1 for single byte character strings, and 2
>>>>> or 4
>>>>> +     for wide characer strings.  */
>>>>> +  unsigned eltsize;
>>>> Bernd's suggestion that we separate the input vs output paramters
>>>> may be
>>>> a reasonable one -- I think this is the only in-parameter you're
>>>> passing
>>>> with the structure, right?  And everything else is a pure output? 
>>>> If so
>>>> we may be better off continuing to pass the element size as a separate
>>>> parameter.
>>>
>>> I changed it in the updated patch.  I had chosen to make it
>>> a member to reduce the number of arguments to these functions and
>>> in anticipation of having them update it before returning if they
>>> discover that the actual element size doesn't match the expected
>>> size, as in:
>>>
>>>    printf ("%ls", "narrow string");
>>>
>>> Similarly to what I proposed here:
>>>    https://gcc.gnu.org/ml/gcc-patches/2018-08/msg01321.html
>>>
>>> I don't see what has been gained by making it an argument again.
>> It's the separation of inputs from outputs he was trying to achieve that
>> I said *may* be a reasonable one.
>>
>> It's not always a good thing, nor is it always a bad thing.  If there
>> are subsequent patches are going to have callees changing it, then it
>> absolutely makes sense to include it.  But adding it to the structure
>> for the mere sake of reducing arguments may not.
>>
>> And just to be clear, I'm not inherently against pulling stuff into
>> structures and classes.   Quite the opposite.
>>
>>>
>>>>> +  /* FLEXARRAY is true if the range of the string lengths has been
>>>>> +     obtained from the upper bound of an array at the end of a
>>>>> struct.
>>>>> +     Such an array may hold a string that's longer than its upper
>>>>> bound
>>>>> +     due to it being used as a poor-man's flexible array member.  */
>>>>> +  bool flexarray;
>>>>> +
>>>>> +  /* Set ELTSIZE and value-initialize all other members.  */
>>>>> +  strlen_data_t (unsigned eltbytes)
>>>>> +    : minlen (), maxlen (), maxsize (), nonstr (), eltsize
>>>>> (eltbytes),
>>>>> +      flexarray () { }
>>>> I think if you pull ELTSIZE out and pass it as a distinct parameter,
>>>> then you don't need a ctor and you can have a POD.  You can then
>>>> initialize with memset rather than having to individually initialize
>>>> each field -- meaning it's easier to add more output fields in the
>>>> future.
>>>
>>> Without ELTSIZE neither a ctor nor memset is necessary for
>>> initialization.  This works too and is the preferred style
>>> in C++ 98:
>>>
>>>    c_strlen_data data = { };
>> I thought that didn't work somewhere.  I certainly would have preferred
>> that over memset when I was twiddling yours and Bernd's earlier patches.
>>
>>
>>
>>>
>>>>
>>>> I don't think any of the suggestions above change the behavior of the
>>>> patch.  Let's hold off committing though (I assume you've got a GIT
>>>> topic branch where you can make these changes and update the subsequent
>>>> patches independent of everything else...)
>>>
>>> I do but I don't know how to make these changes without having
>>> to resolve all the conflicts with the intervening changes on
>>> trunk at each step so I have just a single patch now that resolves
>>> the conflicts with trunk committed since I started this work and
>>> that makes the changes you requested above.  The majority of
>>> the changes in the patch are just minor adjustments (the meat
>>> of the change is in gimple-fold.c; all the rest is just minor
>>> adjustments) so hopefully this won't be a problem.
>> That's a sign you're trying to do too much at once and too long a wait
>> between iterations.
>>
>> Jeff
>>
> 


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]