| Message ID | 705e9f3b-45ab-c91b-0f34-9eb06eb9f0e4@acm.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/7] Preprocessor cleanup | expand |
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 <nathan@acm.org> > > * 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
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
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
On 10/31/18, Marek Polacek <polacek@redhat.com> 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 <nathan@acm.org> >> >> * 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
2018-10-31 Nathan Sidwell <nathan@acm.org> * 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. */