On 11/18/2009 09:20 AM, Martin Sivak wrote:
Hi guys,
I do not really agree with Peter. The macro explodes itself into exactly one line (note the usage of backslashes). So __line__ and __func__ will both be correct. Also I'm pretty sure, that cpp uses line number from the original .c file, not from the preprocessed version of it (all header files are also present in preprocesed version of the .c file, so it would not make sense).
Lowercase naming might be a good idea, but the macro itself seems OK to me.
Martin
----- "David Cantrell"<dcantrell@xxxxxxxxxx> wrote:
This is why I don't like tricky macros.
My first thought on the macro for using asprintf() was "no", but in an
effort
to encourage development, I can put aside my opposition to macros.
But Peter
is right here and I don't use macros enough ever for things like this
to even
notice the problem. Which leads me to now side with him. If we get
in to the
business of having difficult to follow code and confusing macros, it
makes the
code base that much more difficult to maintain down the road.
The macro as edited by Peter is the sort of thing someone might want
to clean
up later on down the road, not realizing we need that construct to
ensure
error reporting is correct.
Also, going this route is trusting the preprocessor for too much
stuff.
Trusting cpp only leads to pain and suffering.
- --
David Cantrell<dcantrell@xxxxxxxxxx>
Martin is right. I tried looking at a cpp output and the macros are
indeed expanded into one line. Besides I tried compiling a small testing
file with a macro, made it fail and observed that the line was reported
exactly right.
I have already replaced the macro name with lowercase version and I am
going to build anaconda (once it is possible for rawhide) and check that
the loader still works as David suggested.
On a higher level: I despise macros as much as you do. I agree that they
decrease the code quality. In our situation however, where the code has
over 70 places where the same four lines exactly repeat, macros are the
lesser evil. Have you noticed that on most places before the patch the
log call looked like this:
logMessage(CRITICAL, "%s: %d: %m", __func__, __LINE__);
There is a new line missing, a failure would dump something like:
critical main: 26: SuccessAborted
Or, consider that the logging mechanism will have to change for some
reason and instead of CRITICAL we will have to write LOG_CRITICAL. This
would be really annoying for someone to fix at all those places. That's
exactly why I think we are trading some less-than-ideal practice for
lower code duplicity and higher maintainability here.
I hope you will agree with that (and with what Martin has said) but if
you don't I won't insist on pushing the patch through of course.
Ales
_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
