Re: [Qemu-ppc] [PATCH for-2.11 2/6] ppc: make cpu_model translation to type consistent

[Igor Mammedov]

On Fri, 25 Aug 2017 23:28:00 +1000
David Gibson <address@hidden> wrote:

> On Fri, Aug 25, 2017 at 01:40:07PM +0200, Igor Mammedov wrote:
> > On Fri, 25 Aug 2017 19:45:38 +1000
> > David Gibson <address@hidden> wrote:
> >   
> > > On Fri, Aug 25, 2017 at 09:27:40AM +0200, Igor Mammedov wrote:  
> > > > On Fri, 25 Aug 2017 11:38:19 +1000
> > > > David Gibson <address@hidden> wrote:
> > > >     
> > > > > On Thu, Aug 24, 2017 at 10:21:47AM +0200, Igor Mammedov wrote:    
> > > > > > PPC handles -cpu FOO rather incosistently,
> > > > > > i.e. it does case-insensitive matching of FOO to
> > > > > > a CPU type (see: ppc_cpu_compare_class_name) but
> > > > > > handles alias names as case-sensitive, as result:
> > > > > > 
> > > > > >  # qemu-system-ppc64 -M mac99 -cpu g3
> > > > > >  qemu-system-ppc64: unable to find CPU model ' kN�U'
> > > > > > 
> > > > > >  # qemu-system-ppc64 -cpu 970MP_V1.1
> > > > > >  qemu-system-ppc64: Unable to find sPAPR CPU Core definition
> > > > > > 
> > > > > > while
> > > > > > 
> > > > > >  # qemu-system-ppc64 -M mac99 -cpu G3
> > > > > >  # qemu-system-ppc64 -cpu 970MP_v1.1
> > > > > > 
> > > > > > start up just fine.
> > > > > > 
> > > > > > Considering we can't take case-insensitive matching away,
> > > > > > make it case-insensitive for  all alias/type/core_type
> > > > > > lookups.
> > > > > > 
> > > > > > As side effect it allows to remove duplicate core types
> > > > > > which are the same but use lower-case letters in name.
> > > > > > 
> > > > > > Signed-off-by: Igor Mammedov <address@hidden>      
> > > > > 
> > > > > Hmm.  So, I'm certainly good with making the matching more consistent,
> > > > > and removing extra entries differing only in case.
> > > > > 
> > > > > However, I don't like capitalizing everything in the model table.  The
> > > > > "canonical" capitalization - as in how it appears in manuals and
> > > > > marketing info - is quite frequently mixed case.  So I think making
> > > > > everything caps in the table will make it harder to read in the
> > > > > context of looking at the corresponding documentation.    
> > > > only cpu models => typenames are made caps, the rest    
> > > 
> > > Yes, I know.  A number of the CPU model names themselves have mixed
> > > case.  Really.
> > >   
> > > > incl. description is kept as is in mixed case.
> > > > It looks like _desc is the thing one would use for
> > > > 1:1 lookup in spec, while _name isn't direct match consistently
> > > > (i.e. sometimes it skips spaces and sometimes spaces are replaced by 
> > > > underscore).
> > > > So keeping _name in mixed case unnecessarily complicates name->type 
> > > > lookup.
> > > > 
> > > > These mixed case + case-insensitive is what made lookup
> > > > code over-engineered and overly complex.    
> > > 
> > > I'm fine with making all the lookups case insensitive.  I just don't
> > > want to drop the case on the names in the actual code.  
> > I've tried to find a way but considering that names are statically
> > 'converted' into typenames and lack of ability to magically
> > uppercase them with preprocessor, I've gave up on this approach.  
> 
> Ok.
> 
> > Providing that there is _desc with model name that should be
> > greppable in spec and complexity of lookup code that mixed-cased
> > typenames incur, I've opted in favor of simplifying lookup
> > code at expense uniform typenames (which is inter QEMU thingy).
> > It is easier to maintain and less chances to make mistake.  
> 
> Yeah, I guess.  It just looks weird to have non-standard capsing in
> the table field.
> 
> Any chance we could standardize on lowercase rather than upper - for
> some reason that would seem less odd to me (I guess because C-ish
> names and identifiers are usually lowercased).
The only reason I've picked up uppercase is that diffstat is smaller
then with lowercase.
So if you prefer lowercase, I'll switch case in v2.
 
[...]