[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[avrdude-dev] Re: code review comments
From: |
Martin Thomas |
Subject: |
[avrdude-dev] Re: code review comments |
Date: |
Thu, 8 Jul 2004 16:26:31 +0200 |
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
- [avrdude-dev] Re: code review comments,
Martin Thomas <=