Andree Leidenfrost said on Wed, Aug 02, 2006 at 08:54:06PM +1000: > > That fixes which one is used (and should be LSB/FSH compliant) and allow > > for some exotic distro to change that conf file only to make it work. > > Definitively for 3.0.x > > I am certainly happy for you to make that design decision. I only > caution that it is a balancing act. If we define too many things > statically in configuration we might end up in some sort of maintenance > hell where we constantly have to adjust configuration to all sorts of > (distribution) changes. (But maybe I am too negative and it will all be > good.)
Well, I think that this maintenance is also something done outside of
the project.
If a new distro wants mondo, and they are not LSB compliant (or LSB
doesn't define something and it's different for them - tyically parted -
then it's way more easy to change one line in an external config file,
rather than to explore all of the code as we do today to solve the issue
!!
So I bet it will reduce our maintenance burden rather than increasing
it.
But that's for 3.0.x and I can't give you code to review right now in
order to help convince ;-)
> > > inptr = (char *)instr;
> >
> > You don't need to cast here.
>
> I was surprised, too. Without the cast I get this with gcc 4.1.2 -Wall:
>
> mr_stresc_demo4.c:17: warning: assignment discards qualifiers from
> pointer target type
Hummm. It makes the code less readable IMO. Casting is normally done
when objects have potential different types, but in fact are the same
(void * being a good example).
I wonder why they add the check between const char * and char *.
In our case, wouldn't it be more simple to remove the const from the
function prototype ? I know it's stupid, but I find more stupid to have
to cast everywhere due to that. It's really makes code unreadable.
> > > *retptr++ = escchr;
> >
> > Rather: *retptr = ESCCHR;
>
> Hm, why limit ourselves unnecessarily? escchr is currently an argument
> to the function, i.e. the escape chararcters could be different. Don;t
> you think that's a good thing?
Well here there is room for discussion.
I was more thinking to make a global define and then use it directly.
If you pass it to the function, then your approach is better. In my case
I was removing one parameter from the call. (I think that the ESC char
will always be \\
But you can keep your approach. It's wider.
What we could then do, is restrict the use in the mr_system function (to
be created)
>
> Interesting. Probably another opportunity to improve my understanding of
> things. Why is a define better than a constant? (Probably dumb but I
> honestly don't know.)
The difference I see, is that defines are handled by the preprocessor,
when variables are handled way later.
Now as said above, it depends on the interface we want.
Keep your previous version for a 3 params func.
> Great. Please find revised new version attached (also escapes " now). If
> you have an idea about the cast I'd be keen - they are still in because
> of the warning mentioned above.
remove the const for me is the way to go.
> > I'd suggest mr_string. for this function.
>
> Right, yeah, that might be a good idea, indeed.
>
> Do you mean mr_string.c and a corresponding mr_string.h?
Yes, I meant mr_string.c.
For the include, I'm not very fond of the way it's done right now
(external, non-external, it's a mess).
For our case, create a mr_string.h describing the interface of the
function, so that when we want to use it, we just #include "mr_string.h"
>
> // Returns the string fed to it 'inptr' with all characters to escape given
> // in 'toesc' prepended by escaping character 'escchr'.
> // (Prepare strings for use in system() or popen() with this function.)
I'll also look at doxygen, as it seems to be already supported by the
rest of the code, to see how to use it really !
> char *mr_stresc(const char *instr, const char *toesc, const char escchr) {
=> char *mr_stresc(char *instr, char *toesc, char escchr)
Bruno.
--
Linux Profession Lead EMEA / Open Source Evangelist \ HP C&I EMEA IET
http://www.mondorescue.org / HP/Intel Solution Center \ http://hpintelco.net
Des infos sur Linux? http://www.HyPer-Linux.org http://www.hp.com/linux
La musique ancienne? http://www.musique-ancienne.org http://www.medieval.org
pgpoUil08REEi.pgp
Description: PGP signature
