On Mon, Feb 10, 2014 at 11:39:51PM +0100, Andreas Färber wrote:
Am 10.02.2014 11:21, schrieb Eduardo Habkost:
+static const TypeInfo x86_cpu_host_type_info = { + .name = CPU_CLASS_NAME("host"), + .parent = TYPE_X86_CPU, + .instance_size = sizeof(X86CPU), + .instance_init = x86_cpu_instance_init_host, + .abstract = false, + .class_size = sizeof(X86CPUClass), + .class_init = x86_cpu_class_init_host, +};
This looks broken: .class_data is not set but the loading of the cpudef happens in the TYPE_X86_CPU initfn. My preferred solution would be to move the cpudef-loading from TYPE_X86_CPU's instance_init to a separate one specified for the models only, allowing non-cpudef-based models. Not finished investigating yet.
class_data is not set because x86_cpu_class_init_host doesn't use it. x86_cpu_class_init_host() simply points fills host_cpu_def and makes xcc->cpu_def point to it. Why would we need a separate instance_init only for the other models? x86_cpu_initfn() already works for everything except the ->features field (that is then handled by x86_cpu_instance_init_host()).
For now I've prepended a patch implementing my generalized CPUClass::class_by_name instead of a custom x86_cpu_class_by_name().
Sounds good to me.
Other style nits that I'm working on cleaning up are declarations in the middle of blocks, keeping _class_init naming convention (pretty sure my patches always had the most-specific-to-least-specific naming), strictly distinguishing between type and class, adding to my gtk-style documentation rather than new custom comments, placing struct documentation in the header and keeping the diff nicely readable AFAP. I'd further like to keep some other conventions from previous CPU subclasses, like pulling the model for loop out of the model registration function.
My patches had always tried to turn what is now x86_cpu_load_def() into an instance_init function rather than calling it from one - did you have reasons not to?
I like to keep the "simply move stuff from X86CPUDefinition to X86CPU" logic in a separate place. I think it makes the code more readable (and it also made the code movements more obvious because I didn't have to move all the code that's inside load_def(), just the load_def() call. But if you want to inline it inside instance_init, I wouldn't mind.
Did you consider converting the host model in a first step to make the patch smaller?
Converting the host model would require moving some logic to instance_init but keeping the strcmp(name, "host") hack. I thought it would involve more complex intermediate steps so I chose not to try it. -- Eduardo