Nathan Sidwell <nat...@acm.org> writes: > On 02/24/16 16:52, Aaron Conole wrote: >> The previous gcov behavior was to always output errors on the stderr channel. >> This is fine for most uses, but some programs will require stderr to be >> untouched by libgcov for certain tests. This change allows configuring >> the gcov output via an environment variable which will be used to open >> the appropriate file. > > this is ok in principle. I have a couple of questions & nits below though.
Thank you for the consideration. I will be submitting a new patch that I hope fully addresses your comments below, either tomorrow or Monday. Thanks so much for the review. > I don't see a previous commit from you -- do you have a copyright > assignment with the FSF? (although this patch is simple, my guess is > the idea it implements is sufficiently novel to need one). We can > handle that off list. I'm happy to report that I did send in some FSF paperwork this week. Hopefully it is on record now, but even if it isn't I live a train ride away from the FSF headquarters so I'd be happy to take the time to make sure it's all signed correctly. >> diff --git a/libgcc/libgcov-driver-system.c b/libgcc/libgcov-driver-system.c >> index 4e3b244..0eb9755 100644 >> --- a/libgcc/libgcov-driver-system.c >> +++ b/libgcc/libgcov-driver-system.c >> @@ -23,6 +23,24 @@ a copy of the GCC Runtime Library Exception along >> with this program; >> see the files COPYING3 and COPYING.RUNTIME respectively. If not, see >> <http://www.gnu.org/licenses/>. */ >> >> +FILE *__gcov_error_file = NULL; > > Unless I'm missing something, isn't this only accessed from this file? > (So could be static with a non-underbarred name) Ack. >> @@ -30,12 +48,27 @@ gcov_error (const char *fmt, ...) >> { >> int ret; >> va_list argp; >> + >> + if (!__gcov_error_file) >> + __gcov_error_file = get_gcov_error_file(); > > Needs space before () Ack. >> + >> va_start (argp, fmt); >> - ret = vfprintf (stderr, fmt, argp); >> + ret = vfprintf (__gcov_error_file, fmt, argp); >> va_end (argp); >> return ret; >> } >> >> +#if !IN_GCOV_TOOL > > And this protection here, makes me wonder what happens if one is > IN_GCOV_TOOL. Does it pay attention to GCOV_ERROR_FILE? That would > seem incorrect, and thus the above should be changed so that stderr is > unconditionally used when IN_GCOV_TOOL? You are correct. I will fix it. >> +static void >> +gcov_error_exit(void) >> +{ >> + if (__gcov_error_file && __gcov_error_file != stderr) >> + { > > Braces are not needed here. Ack. >> --- a/libgcc/libgcov-driver.c >> +++ b/libgcc/libgcov-driver.c >> @@ -46,6 +46,10 @@ void __gcov_init (struct gcov_info *p >> __attribute__ ((unused))) {} > >> + gcov_error_exit(); > > Needs space before (). Ack. > nathan
