On Thu, Feb 09, 2017 at 09:52:03 +0100, Michal Privoznik wrote: > On 02/08/2017 01:43 PM, Peter Krempa wrote: > > On Wed, Feb 08, 2017 at 13:37:48 +0100, Michal Privoznik wrote: > >> On 02/08/2017 01:23 PM, Peter Krempa wrote: > >>> On Wed, Feb 08, 2017 at 11:37:07 +0100, Michal Privoznik wrote: > >>>> Nearly all of these functions look the same. Except for a > >>>> different virSecurityManager API call. There is no need to copy > >>>> paste the code when we can use macros to generate it. > >>>> > >>>> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx> > >>>> --- > >>>> src/qemu/qemu_security.c | 179 ++++++++++++----------------------------------- > >>>> 1 file changed, 44 insertions(+), 135 deletions(-) > >>> > >>> NACK, please don't partialy define function with macros. > >>> > >> > >> Why not? What is the downside? > > > > You'll never be able to navigate to the body of the function or ever > > find it try 'vim -t qemuSecurityRestoreHostdevLabel' or navigate to > > that after that patch. > > > > The downside of the code being totally unreadable is way worse than a > > few copied lines. > > > > (YU|NA)CK > > > > VIR_ONCE_GLOBAL_INIT I've hit these a few times. In this case it irritates me that I can't see the ...Initialize function. > KVM_FEATURE_DEF This does not create functions. > DECLARE_CLASS_COMMON This does not create functions. The variables accessed are global and declared without any macro All usage is within one screen. Only opaque function call is to the ...Dispose function. All functions can be viewed with vim -t. The macros are undefined within the single case where it's used. This does not help your example. > src/esx/* Don't care, never used ESX. > tests/* Granted, in tests there are quite a few questionable usages of macros. One of the worst examples is TEST_CHAIN. Tests are usually "fun" to debug. > XDR_PRIMITIVE_DISSECTOR Don't care, never used the dissector really. > VIR_ENUM_DECL > VIR_ENUM_IMPL I'm hitting these regularly, but they are so common that I got used to it and also it's pretty easy to figure out the type name you want to access. Seeing the code in this case would not help that much though. It's far more useful to see the enum or the strings belonging to it. The only option I see here is if we'd generate them into a separate file rather than relying on macros. But as I've said that would not help much since you want to see the strings. > PROBE This macro uses __LINE__ and __FILE__ stuff so it can't be avoided. Also if you don't enable DTRACE the macro does not expand to anything, which is the default. > just to name a few places where we already do what you NACKed. If using > macros to construct functions is that bad, I expect you to write a patch > to get rid of those. No I won't fix those just because I rejected your code. Reasoning that something should be allowed because we have prior art is weak. We don't have to make the code uglier than it is.
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
