On 08/25/18 21:02, Jeff Law wrote:
> On 08/25/2018 12:36 PM, Bernd Edlinger wrote:
>
>>>>>
>>>>
>>>> Well, ya call it "layer one patch over the other"
>>>> I call it "incremental improvements".
>>> It is (of course) a case by case basis. The way I try to look at these
>>> things is to ask whether or not the first patch under consideration
>>> would have any value/purpose after the second patch was installed. If
>>> so, then it may make sense to include both. If not, then we really just
>>> want one patch.
>>>
>>
>> Agreed. I think the question is which of the possible STRING_CST
>> semantics we want to have in the end (the middle-end).
>> Everything builds on top of the semantic properties of STRING_CSTs.
> This certainly plays a role. I bumped pretty hard against the
> STRING_CST semantics issue with Martin's patch. I'm hoping that making
> those more consistent will ultimately simplify things and avoid the
> problems I'm stumbling over.
>
> Of course, that means more delays in getting this sorted out. I really
> thought I had a viable plan a couple days ago, but I'm having to rethink
> in light of some of the issues raised.
>
I think we should slow down.
>
>>
>> My first attempt of fix the STRING_CST semantic was trying to make
>> string_constant happy.
>>
>> My second attempt is trying to make Richard happy. And when I look
>> at both patches, I think the second one is better, and more simple.
> In general I've found that Richie's advice generally results in a
> cleaner implementation ;-)
>>
>>
>> BTW I need to correct on statement in my last e-mail:
>>
>> On 08/24/18 23:55, Bernd Edlinger wrote:>
>>> here I quote form martins 1/6 patch:
>>>> + /* Compute the lower bound number of elements (not bytes) in the array
>>>> + that the string is used to initialize. The actual size of the array
>>>> + will be may be greater if the string is shorter, but the important
>>>> + data point is whether the literal, including the terminating nul,
>>>> + fits in the array. */
>>>> + unsigned HOST_WIDE_INT array_elts
>>>> + = tree_to_uhwi (TYPE_SIZE_UNIT (TREE_TYPE (init))) / charsize;
>>>> +
>>>> + /* Compute the string length in (wide) characters. */
>>>
>>> So prior to my STRING_CST patch this will ICE on a flexible array member,
>>> because those have TYPE_SIZE_UNIT = NULL, and tree_to_uhwi will ICE.
>>>
>>> I used:
>>>
>>> compare_tree_int (TYPE_SIZE_UNIT (TREE_TYPE (init)),
>>> TREE_STRING_LENGTH (init))
>>>
>>> and this will not ICE with NULL, but consider it like infinity,
>>> and return 1.
>>>
>>> So my version did not ICE in that case.
>>>
>>
>> Oooops,
>>
>> actually both versions will likely ICE on TYPE_SIZE_UNIT == NULL.
>> I actually tried to test that case, but have done something wrong.
>>
>> I hope we can get rid if the incomplete types in the middle-end.
>>
>> So maybe just inject "if (!tree_fits_uhwi_p (...)) return NULL_TREE;"
>> here? Or maybe just defer until we have clarity about the semantics.
> Not sure. I've tried largely to not let VLA issues drive anything here.
> I'm not a fan of them for a variety of reasons and thus I tend to look
> at all the VLA stuff as exceptional cases.
>
We should have a test case with flexible array members, those behave
slightly different than VLAs.
struct {
int i;
char x[];
} s;
const struct s s = { 1, "test" };
int f()
{
return strlen(s.x);
}
By the way, when you change so much on Martin's patch it might be
good to post it again on this list, when it's ready, so maybe I can have a
look at it and help out with some review comments? (please put me on CC)
Bernd.
