On Tue, 22 Dec 2015 16:47:18 +0200
"Michael S. Tsirkin" <m...@redhat.com> wrote:
> On Tue, Dec 22, 2015 at 03:38:24PM +0100, Igor Mammedov wrote:
> > On Tue, 22 Dec 2015 11:37:40 +0200
> > "Michael S. Tsirkin" <m...@redhat.com> wrote:
> >
> > [...]
> > > > > > + for (i = 4; i <= 0xF; i++) {
> > > > > > + char *name = g_strdup_printf("_L0%X", i);
> > > > > > + method = aml_method(name, 0, AML_NOTSERIALIZED);
> > > > > > + aml_append(scope, method);
> > > > > > + g_free(name);
> > > > > > + }
> > > > >
> > > > > How about we make aml_method accept ... format instead?
> > > > actually instead of making aml_method(format,...) it would be
> > > > easier to make it accept Aml* so we could reuse aml_name(format,...)
> > > > in the end it would look like:
> > > >
> > > > Aml gpe_name = aml_name("_L0%X", i);
> > > > aml_method(gpe_name, AML_NOTSERIALIZED);
> > > >
> > > > in addition name object could be reused in other places
> > > > that reference that method.
> > >
> > > Except most methods are simple, so maybe do both APIs.
> > > Avoiding string duplication is a good idea,
> > > but using string constants works just as well.
> > I don't like having 2 APIs, one with 'Aml* name' and other
> > with 'const char *name' in C. I'd prefer to have just one
> > that suits the majority use cases and
> >
> > I'm not so comfortable with using format string here directly
> > as it would look weird (at least to me):
> > aml_method("_L0%X", i, argcount, AML_NOTSERIALIZED);
> > - static format string check at compile time won't work here
> > or
> > aml_method(argcount, AML_NOTSERIALIZED, "_L0%X", i);
> > - that should be fine except of that argument order doesn't
> > match the way it's in spec, which I'd prefer to keep
>
> Just add ... at the end:
>
> aml_method("_L0%X", argcount, AML_NOTSERIALIZED, i);
ok, I'll try to do it.
>
> looks a bit weird if you have to use it, but
> it's uncommon, and the common case looks simple.
>
> > so it leaves for me 2 options:
> > 1: use aml_method(aml_name("_L0%X", i), argcount, AML_NOTSERIALIZED)
> > It's a bit of code churn and not sure if we really need it
> > but I can do if asked for it.
> >
> > 2: just leave it as is for now, because the most of method names
> > are just string constants. These "_L0%X" will be gone after
> > cleanup, leaving us with only handlers that use string const and
> > do some work.
> >
> > I can replace g_strdup_printf() with static buffer here if you
> > dislike alloc/free in this patch.
>
> So how about just open-coding this loop for now?
> That's just about 10 lines of code,
> not a big deal.
sure
>
> As you say, will be gone after refactoring.
>
