Re: [5/6] Preprocessor include

[Eric Gallager]

On 10/31/18, Marek Polacek  wrote:
> On Wed, Oct 31, 2018 at 11:02:16AM -0400, Nathan Sidwell wrote:
>> [Yes, I can't count]
>>
>> Include file handling duplicated cleanup code in each exit path. Simpler
>> to
>> just commonize it with  goto.  Also noticed a memory leak in buffer
>> popping.
>>
>> Applying to trunk.
>>
>> --
>> Nathan Sidwell
>
>> 2018-10-31  Nathan Sidwell  
>>
>>  * directives.c (do_include_common): Commonize cleanup path.
>>  (_cpp_pop_buffer): Fix leak.
>>
>> Index: libcpp/directives.c
>> ===
>> --- libcpp/directives.c  (revision 265671)
>> +++ libcpp/directives.c  (working copy)
>> @@ -827,22 +827,15 @@ do_include_common (cpp_reader *pfile, en
>>
>>fname = parse_include (pfile, &angle_brackets, &buf, &location);
>>if (!fname)
>> -{
>> -  if (buf)
>> -XDELETEVEC (buf);
>> -  return;
>> -}
>> +goto done;
>>
>>if (!*fname)
>> -  {
>> -cpp_error_with_line (pfile, CPP_DL_ERROR, location, 0,
>> - "empty filename in #%s",
>> - pfile->directive->name);
>> -XDELETEVEC (fname);
>> -if (buf)
>> -  XDELETEVEC (buf);
>> -return;
>> -  }
>> +{
>> +  cpp_error_with_line (pfile, CPP_DL_ERROR, location, 0,
>> +   "empty filename in #%s",
>> +   pfile->directive->name);
>> +  goto done;
>> +}
>>
>>/* Prevent #include recursion.  */
>>if (pfile->line_table->depth >= CPP_STACK_MAX)
>> @@ -860,6 +853,7 @@ do_include_common (cpp_reader *pfile, en
>>_cpp_stack_include (pfile, fname, angle_brackets, type, location);
>>  }
>>
>> + done:
>>XDELETEVEC (fname);
>>if (buf)
>>  XDELETEVEC (buf);
>> @@ -2618,6 +2612,8 @@ _cpp_pop_buffer (cpp_reader *pfile)
>>
>>_cpp_do_file_change (pfile, LC_LEAVE, 0, 0, 0);
>>  }
>> +  else if (to_free)
>> +free ((void *)to_free);
>
> free (NULL) is ok so do you really need the check?
>
> Marek
>

This is bug 80528 by the way: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80528

Re: [5/6] Preprocessor include

[Jakub Jelinek]

On Wed, Oct 31, 2018 at 12:24:44PM -0400, Nathan Sidwell wrote:
> On 10/31/18 11:44 AM, Marek Polacek wrote:
> 
> > > +  else if (to_free)
> > > +free ((void *)to_free);
> > 
> > free (NULL) is ok so do you really need the check?
> 
> Probably not :)  IIRC the if (X) free (X) idiom was nearby, and I didn't
> feel like rocking that boat.

The if (x) free (x); idiom is useful if x is unlikely or very unlikely and
the cost of the function call is measureable.  But one should probably
use __builtin_expect in that case...

Jakub

Re: [5/6] Preprocessor include

[Nathan Sidwell]

On 10/31/18 11:44 AM, Marek Polacek wrote:


+  else if (to_free)
+free ((void *)to_free);


free (NULL) is ok so do you really need the check?


Probably not :)  IIRC the if (X) free (X) idiom was nearby, and I didn't 
feel like rocking that boat.


nathan

--
Nathan Sidwell

Re: [5/6] Preprocessor include

[Marek Polacek]

On Wed, Oct 31, 2018 at 11:02:16AM -0400, Nathan Sidwell wrote:
> [Yes, I can't count]
> 
> Include file handling duplicated cleanup code in each exit path. Simpler to
> just commonize it with  goto.  Also noticed a memory leak in buffer popping.
> 
> Applying to trunk.
> 
> -- 
> Nathan Sidwell

> 2018-10-31  Nathan Sidwell  
> 
>   * directives.c (do_include_common): Commonize cleanup path.
>   (_cpp_pop_buffer): Fix leak.
> 
> Index: libcpp/directives.c
> ===
> --- libcpp/directives.c   (revision 265671)
> +++ libcpp/directives.c   (working copy)
> @@ -827,22 +827,15 @@ do_include_common (cpp_reader *pfile, en
>  
>fname = parse_include (pfile, &angle_brackets, &buf, &location);
>if (!fname)
> -{
> -  if (buf)
> - XDELETEVEC (buf);
> -  return;
> -}
> +goto done;
>  
>if (!*fname)
> -  {
> -cpp_error_with_line (pfile, CPP_DL_ERROR, location, 0,
> -  "empty filename in #%s",
> -  pfile->directive->name);
> -XDELETEVEC (fname);
> -if (buf)
> -  XDELETEVEC (buf);
> -return;
> -  }
> +{
> +  cpp_error_with_line (pfile, CPP_DL_ERROR, location, 0,
> +"empty filename in #%s",
> +pfile->directive->name);
> +  goto done;
> +}
>  
>/* Prevent #include recursion.  */
>if (pfile->line_table->depth >= CPP_STACK_MAX)
> @@ -860,6 +853,7 @@ do_include_common (cpp_reader *pfile, en
>_cpp_stack_include (pfile, fname, angle_brackets, type, location);
>  }
>  
> + done:
>XDELETEVEC (fname);
>if (buf)
>  XDELETEVEC (buf);
> @@ -2618,6 +2612,8 @@ _cpp_pop_buffer (cpp_reader *pfile)
>  
>_cpp_do_file_change (pfile, LC_LEAVE, 0, 0, 0);
>  }
> +  else if (to_free)
> +free ((void *)to_free);

free (NULL) is ok so do you really need the check?

Marek

[5/6] Preprocessor include

[Nathan Sidwell]

[Yes, I can't count]

Include file handling duplicated cleanup code in each exit path. 
Simpler to just commonize it with  goto.  Also noticed a memory leak in 
buffer popping.


Applying to trunk.

--
Nathan Sidwell
2018-10-31  Nathan Sidwell  

	* directives.c (do_include_common): Commonize cleanup path.
	(_cpp_pop_buffer): Fix leak.

Index: libcpp/directives.c
===
--- libcpp/directives.c	(revision 265671)
+++ libcpp/directives.c	(working copy)
@@ -827,22 +827,15 @@ do_include_common (cpp_reader *pfile, en
 
   fname = parse_include (pfile, &angle_brackets, &buf, &location);
   if (!fname)
-{
-  if (buf)
-	XDELETEVEC (buf);
-  return;
-}
+goto done;
 
   if (!*fname)
-  {
-cpp_error_with_line (pfile, CPP_DL_ERROR, location, 0,
-			 "empty filename in #%s",
-			 pfile->directive->name);
-XDELETEVEC (fname);
-if (buf)
-  XDELETEVEC (buf);
-return;
-  }
+{
+  cpp_error_with_line (pfile, CPP_DL_ERROR, location, 0,
+			   "empty filename in #%s",
+			   pfile->directive->name);
+  goto done;
+}
 
   /* Prevent #include recursion.  */
   if (pfile->line_table->depth >= CPP_STACK_MAX)
@@ -860,6 +853,7 @@ do_include_common (cpp_reader *pfile, en
   _cpp_stack_include (pfile, fname, angle_brackets, type, location);
 }
 
+ done:
   XDELETEVEC (fname);
   if (buf)
 XDELETEVEC (buf);
@@ -2618,6 +2612,8 @@ _cpp_pop_buffer (cpp_reader *pfile)
 
   _cpp_do_file_change (pfile, LC_LEAVE, 0, 0, 0);
 }
+  else if (to_free)
+free ((void *)to_free);
 }
 
 /* Enter all recognized directives in the hash table.  */