incubator-stdcxx-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Martin Sebor <se...@roguewave.com>
Subject Re: [PATCH]Assorted cleanup
Date Fri, 10 Nov 2006 19:28:32 GMT
Andrew Black wrote:
[...]
> This is a somewhat cosmetic change, but it does allow the exec utility 
> to run if the library were to build incorrectly such that it crashes 
> during initialization.
> 
> exec_unlink.diff Changelog:
>     * GNUmakefile.bin (LDFLAGS.exec): Define LDFLAGS variant, filtering 
> out the stdcxx library
>       (exec): use LDFLAGS.exec rather than LDFLAGS

Okay. Please do try to capitalize/terminate your sentences in
the ChangeLog. ChangeLogs deserve the same attention to detail
as the code changes they document.

> 
[...]
> I still feel that it is more efficient to assign the signal handler 
> reference to the element in the sigaction structure rather than using 
> memcpy, but I also agree that the use of the typedef is cleaner than 
> using the #ifndef logic.

Directly assigning the pointer probably is going to be more
efficient in most cases (some optimizers, such as gcc, manage
to eliminate the memcpy call and replace it with the equivalent
all over of the plain assignment, but others, such as Sun C++,
do not). But the point of the memcpy call here is to avoid the
incompatibility and to work around the HP aCC bug. We don't
need to be that concerned with efficiency in this code, so I
wouldn't lose sleep over it if I were you :)

> 
> compaqfix.diff Changelog:
>     * exec.cpp (alarm_handler) [!_WIN32 && !_WIN64]:

There is no need for the &&. .. part here or in code as the _WIN32
macro is guaranteed to be defined. Please stop using it -- it only
makes the text harder to read. Other than that, if this compiles
with aCC 6 and Compaq C++ 7 go ahead and commit it.

Thanks
Martin

  Define typedef with
> signature matching that of handle_alrm.
>       (wait_for_child) [!_WIN32 && !_WIN64]: Use alarm_handler typedef 
> for type of local variable storing reference to handle_alrm.
> 
> --Andrew Black
> 
> 
> ------------------------------------------------------------------------
> 
> Index: etc/config/GNUmakefile.bin
> ===================================================================
> --- etc/config/GNUmakefile.bin	(revision 472548)
> +++ etc/config/GNUmakefile.bin	(working copy)
> @@ -45,6 +45,9 @@
>    LDFLAGS += $(CPPFLAGS)
>  endif   # ($(CXX_REPOSITORY),)
>  
> +# Don't want to link exec utility with stdlib, so create our own LDFLAGS var
> +LDFLAGS.exec = $(filter-out -l$(LIBBASE),$(LDFLAGS))
> +
>  ##############################################################################
>  #  TARGETS
>  ##############################################################################
> @@ -55,8 +58,8 @@
>  
>  # link the run utility
>  exec: runall.o cmdopt.o output.o util.o exec.o display.o
> -	@echo "$(LD) $^ -o $@ $(LDFLAGS) $(LDLIBS)" >> $(LOGFILE)
> -	$(LD) $^ -o $@ $(LDFLAGS) $(LDLIBS) $(TEEOPTS)
> +	@echo "$(LD) $^ -o $@ $(LDFLAGS.exec) $(LDLIBS)" >> $(LOGFILE)
> +	$(LD) $^ -o $@ $(LDFLAGS.exec) $(LDLIBS) $(TEEOPTS)
>  
>  # link the localedef utility
>  localedef: localedef.o aliases.o charmap.o codecvt.o collate.o ctype.o \
> 
> 
> ------------------------------------------------------------------------
> 
> Index: exec.cpp
> ===================================================================
> --- exec.cpp	(revision 472548)
> +++ exec.cpp	(working copy)
> @@ -360,6 +360,8 @@
>          alarm_timeout = 1;
>  }
>  
> +typedef void (*alarm_handler)(int);
> +
>  #ifdef __cplusplus
>  }
>  #endif
> @@ -421,7 +423,7 @@
>      /* avoid extern "C"/"C++" mismatch due to an HP aCC 6 bug
>         (see STDCXX-291)
>      */
> -    void (*phandler)(int) = handle_alrm;
> +    alarm_handler phandler = handle_alrm;
>      memcpy (&act.sa_handler, &phandler, sizeof act.sa_handler);
>  
>      sigaction (SIGALRM, &act, 0);


Mime
View raw message