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
