Re: [PATCH] preprocessor/58580 - preprocessor goes OOM with warning for zero literals

[Dodji Seketeli <dodji@redhat.com>]

Jakub Jelinek <jakub@redhat.com> writes:

>> -OBJS-libcommon = diagnostic.o diagnostic-color.o pretty-print.o intl.o input.o version.o
>> +OBJS-libcommon = diagnostic.o diagnostic-color.o pretty-print.o intl.o vec.o input.o version.o
>
> Too long line?

Fixed in my local copy of the patch, thanks.

>
>> +      if (c == '\0')
>> +	c = ' ';
>>        pp_character (context->printer, c);
>
> Does that match how libcpp counts the embedded '\0' character in column
> computation?

Yes, I think so.  _cpp_lex_direct in libcpp/lex.c considers '\0' just
like a white space and so the column number is incremented when it's
encountered.

>> +    /* The position (byte count) the the last byte of the line.  This
>> +       normally points to the '\n' character, or to one byte after the
>> +       last byte of the file, if the file doesn't contain a '\n'
>> +       character.  */
>> +    size_t end_pos;
>
> Does it really help to note this?  You can always just walk the line from
> start_pos looking for '\n' or end of file.

Yes you are right, it's not strictly necessary.  But with that end_pos,
copying a line is even faster; no need of walking.  I thought the goal
was to avoid re-doing the work we've already done, as much as possible.

>
>> +static fcache*
>> +add_file_to_cache_tab (const char *file_path)
>> +{
>> +  static size_t idx;
>> +  fcache *r;
>> +
>> +  FILE *fp = fopen (file_path, "r");
>> +  if (ferror (fp))
>> +    {
>> +      fclose (fp);
>> +      return NULL;
>> +    }
>> +
>> +  r = &fcache_tab[idx];
>
> Wouldn't it be better to use some LRU algorithm to determine which
> file to kick out of the cache?  Have some tick counter of cache lookups (or
> creation) and assign the tick counter to the just created resp. used
> cache entry, and remove the one with the smallest counter?

Hehe, the LRU idea occurred to me too, but I dismissed the idea as
something probably over-engineered.  But now that you are mentioning it
I guess I should give it a try ;-) I'll post a patch about that later
then.

>> +  fcache * r = lookup_file_in_cache_tab (file_path);
>> +  if (r ==  NULL)
>
> Formatting (no space after *, extra space after ==).

Fixed in my local copy.  Thanks.

-- 
		Dodji