[avrdude-dev] Re: code review comments

[Martin Thomas]

Hi,

some comments to the previous messages:

---------------------

*** Date: Mon, 5 Jul 2004 23:04:19 +1200
From: "Alex Shepherd"

>> Some additional changes for avrdude (based on
>> a cvs-checkout from Jun, 29 2004)
>Maybe you should be added to the developer list so you can add commit
>changes?

My cvs knowledge is limited and I do not use Linux/Unix-platform 
"on the desktop" so I can't test if my changes break the build-process
on this platforms.


---------------------

Date: Wed, 7 Jul 2004 15:38:12 -0700 (PDT)
From: "Theodore A. Roth"

>  # See if we need to drop into the windows subdir.
>  case $target in
> -        *-*-mingw32* | *-*-cygwin* | *-*-windows*)
> +        *-*-cygwin*)
> + WINDOWS_DIRS="windows"
> + LDFLAGS="-static"
> + ;;
> +        *-*-mingw32* | *-*-windows*)
>   WINDOWS_DIRS="windows"
>   CFLAGS="-mno-cygwin -DWIN32NATIVE"
>   LDFLAGS="-static"

If I got this right, Alex removed several "#ifdef(CYGIN)". I don't
know if the cygwin-target will compile/link correctly any longer.

---------------------

*** Date: Wed, 07 Jul 2004 17:04:19 -0600
From: "E. Weddington" 

> One can use the Cygwin environment (shell and programs) to build the 
> application. The application can be built in two separate ways:
> 
> 1. One can "build for Cygwin", which implies that the resulting application 
> is 
> linked to Cygwin DLLs.
> 2. One can "build for MinGW", which implies that the resulting application is 
> NOT linked to Cygwin DLLs and is completely a native Win32 application. *Even 
> though* we're using the Cygwin environment to build it.
> 
> The -mno-cygwin flag is the standard way of switching over to the MinGW GCC  
> compiler (that can come as a Cygwin package) and build and link the 
> application 
> as a Win32 application. This flag is required if we're going to be using the 
> Win32 calls.

I can't think of a situation where someone would build an avrdude.exe for WIN32
target-platforms which is depending on the cygwin-runtime-dll. Now that the 
"dll-free"
build is possible maybe the cygwin-target could be dropped and MinGW-target
(=no dll) might be the only Win32-target.

Even inside the cygwin-shell avrdude.exe should work without problems (A lot
of testing during development has been done from "inside" cygwin). The
only drawback might be, that makefiles created for Unix/BSD-Systems which 
call avrdude have to be modified (path/dev) since avrdudeW32 is not
connected to the "unix-comaptibility-layer" provided by the cygwin-dll 
any longer.

Ah - and readline is not supported in terminal-mode on W32 so far, but 
I've seen a compatible readline-lib somewhere in MinGW/MinSYS. Maybe 
a "multi-platform" readline could be included as source-code in the avrdude 
code-tree

---------------------

***Date: Wed, 7 Jul 2004 16:06:52 -0700 (PDT)
From: "Theodore A. Roth" 

> I'm looking through the code a bit and have a few comments.
> 
> In main():
> 
>     905     else {
>     906       update_progress = update_progress_no_tty;
>     907       #if defined(WIN32NATIVE)
>     908       /* disable all buffering of stderr for compatibility with
>     909          software that captures and redirects output to a GUI
>     910          i.e. Programmers Notepad */
>     911       setvbuf( stderr, NULL, _IONBF, 0 );
>     912       setvbuf( stdout, NULL, _IONBF, 0 );
>     913       #endif /* WIN32NATIVE */
>     914         }
> 
> Could lines 907 and 913 just be removed? I doubt this would hurt for
> other systems and might even help.

according to "man 3 setvbuf" on a debian-maschine setvbuf "conforms
to ANSI X3.159-1989 so dropping lines 907/913 should not break anything

---------------------

***Date: Wed, 07 Jul 2004 17:21:23 -0600
From: "E. Weddington"

> > Is the native w32 still experimental? Can that be removed?
> 
> It should be removed. It's been tested enough IMHO.

There has been one "issue" from someone who had problems 
programming locks/fuses an a 16MHz/ATmega16 reported on the
list. I can not reproduce this problem here - might be a hardware 
problem. 

Another problem was in the butterfly-programmer 
(AVR-109/910-type-bootloader) in the Win32-native-build. It seems
that the 4.3.0-code works on *nix/Cywin but fails on W32native.
I've sent a patch to fix this on W32 in a  previous mail to the list.
Maybe someone owning a BF and using *nix can test if the patch
breaks the Unix/BSD-target-version (Joerg?) and commit it 
to the CVS.

---------------------

*** Date: Wed, 7 Jul 2004 16:32:27 -0700 (PDT)
From: "Theodore A. Roth"

> > > Shouldn't all the leading '#' characters for preprocessor directives be
> > > at the beginning of the line instead of intended? Does indenting those
> > > work with all compilers on all systems?
> >
> > Considering all the systems that avrdude is known to work on, the
> > compiler that is being used on all those systems is GCC, and no other.
> > Indenting preprocessor directives is known to work on GCC. So,
> > technically it's fine. But if somebody (Brian) wants to declare a
> > stylistic preference for the application, that's a different matter.
> 
> Personnally, I'd rather they were not indented. They are harder to
> notice when indented and are usually code that need more careful looking
> over since the code is compiled differently based on the defines. They
> should really stick out like a sore thumb so that they get noticed and
> scrutinized. Indenting them also confuses some editor when coloring the
> syntax further hiding them. I've never seen any code with them indented
> either.

Since I'm the one who inserted some of these indended lines:
Personnally, I prefer the intented form if the directives do not change
the program-flow. I just 'looks nicer' and not flattered (sp?). But o.k.,
since the uninteded form is more compatible the tabs should be removed.




Martin