Subject: Re: [PATCH] Etags support for Mercury -- fix explicit tags for existentially-quantified procedures

From: fabrice nicol <[email-masked]>
Date: Mon, 14 Jun 2021 19:10:53 +0200

Yes, I'm afraid that you missed something there this time around.

If you take a look at other languages that implement explicit tags (like 
Fortran) you will see that there are buffers coming in to avoid having 
'name' and 'linestart' (1st and 4th arguments to 'make_tag') share a 
same string pointer (here 's').

This is explained in the header comment to 'make_tag':

" 2. LINESTART contains name as either a rightmost, or rightmost but
  *     one character, substring;"

which is a bit of a convoluted constraint: better protect oneself and 
bufferize from the ground up.

In most cases (though perhaps with occasional exceptions), if I followed 
your suggestion, 'name' would be a substring with aon offset of  at 
least +2 bytes from start of string s.

When I applied you suggestion and tested, the TAGS base was as expected 
accordingly: wrong.

Fabrice

>> Thanks.  I didn't yet try to apply and run the patch, but one aspect
>> of the patch caused me to raise mu brow:
>>
>> +      char *name = xnew (pos + 1, char);
>> +      size_t namelength = position.namelength;
>> +      if (stop_at_rule && offset) --offset;
>> +
>> +      /* Left-trim type definitions.  */
>> +
>> +      while (pos > namelength + offset
>> +	     && c_isspace (s[pos - namelength - offset]))
>> +	--offset;
>> +
>> +      memcpy (name, s + pos - namelength - offset, namelength);
>> +
>> +      /* There is no need to correct namelength or call notinname.  */
>> +      name[namelength - 1] = '\0';
>> +
>> +      make_tag (name, namelength, true, s, pos, lineno, linecharno);
>> +      free (name);
> Why do you copy the identifier's name into a newly-allocated buffer,
> instead of just passing 's + pos - namelength - offset' and
> 'namelength' as the first 2 arguments of make_tag?  Isn't this
> xnew+memcpy+free dance here redundant?  Or what did I miss?