----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/873/#review1547 -----------------------------------------------------------
Were there places where the ambiguity between a structure field access and the type notation were giving you problems? Would they have been fixed just by making the one change here where the regex matches only valid type extensions and not any sequence of characters after a dot? My reluctance here is that you're substituting one ambiguous overloaded syntax for another... it's unlikely that I have an identifier that by itself ends in "_uq" but it seems equally unlikely that I've got a structure field named "uq". Between the two, IMO, the dot syntax is slightly more meaningful since the type qualifiers are a little bit like union fields. It's fine to me to change it, but it seems like we should change it to something that's clearly less ambiguous; how about colon? There's some potential ambiguity with ?:, but that's not a very common operator, and even when there's potential ambiguity it would be easy to fix by requiring spaces around the : when you don't want it to be treated as a type qualifier. Some other possibilities are #, `, \, ~, and !. I think only backtick is completely unused, but the others should be pretty unambiguous when embedded in an identifier. - Steve On 2011-09-20 22:59:24, Gabe Black wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/873/ > ----------------------------------------------------------- > > (Updated 2011-09-20 22:59:24) > > > Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and > Nathan Binkert. > > > Summary > ------- > > ISA parser: Use '_' instead of '.' to delimit type modifiers on operands. > > By using an underscore, the "." is still available and can unambiguously be > used to refer to members of a structure if an operand is a structure, class, > etc. This change mostly just replaces the appropriate "."s with "_"s, but > there were also a few places where the ISA descriptions where handling the > extensions themselves and had their own regular expressions to update. The > regular expressions in the isa parser were updated as well. It also now > looks for one of the defined type extensions specifically after connecting "_" > where before it would look for any sequence of characters after a "." > following an operand name and try to use it as the extension. This helps to > disambiguate cases where a "_" may legitimately be part of an operand name but > not separate the name from the type suffix. > > Because leaving the "_" and suffix on the variable name still leaves a valid > C++ identifier and all extensions need to be consistent in a given context, I > considered leaving them on as a breadcrumb that would show what the intended > type was for that operand. Unfortunately the operands can be referred to in > code templates, the Mem operand in particular, and since the exact type of Mem > can be different for different uses of the same template, that broke things. > > > Diffs > ----- > > src/arch/alpha/isa/decoder.isa dee1f3ab92e4 > src/arch/alpha/isa/int.isa dee1f3ab92e4 > src/arch/alpha/isa/main.isa dee1f3ab92e4 > src/arch/arm/isa/insts/branch.isa dee1f3ab92e4 > src/arch/arm/isa/insts/data.isa dee1f3ab92e4 > src/arch/arm/isa/insts/div.isa dee1f3ab92e4 > src/arch/arm/isa/insts/fp.isa dee1f3ab92e4 > src/arch/arm/isa/insts/ldr.isa dee1f3ab92e4 > src/arch/arm/isa/insts/macromem.isa dee1f3ab92e4 > src/arch/arm/isa/insts/mem.isa dee1f3ab92e4 > src/arch/arm/isa/insts/misc.isa dee1f3ab92e4 > src/arch/arm/isa/insts/mult.isa dee1f3ab92e4 > src/arch/arm/isa/insts/neon.isa dee1f3ab92e4 > src/arch/arm/isa/insts/str.isa dee1f3ab92e4 > src/arch/arm/isa/insts/swap.isa dee1f3ab92e4 > src/arch/isa_parser.py dee1f3ab92e4 > src/arch/mips/isa/decoder.isa dee1f3ab92e4 > src/arch/mips/isa/formats/fp.isa dee1f3ab92e4 > src/arch/mips/isa/formats/int.isa dee1f3ab92e4 > src/arch/mips/isa/formats/mem.isa dee1f3ab92e4 > src/arch/power/isa/decoder.isa dee1f3ab92e4 > src/arch/power/isa/formats/fp.isa dee1f3ab92e4 > src/arch/sparc/isa/base.isa dee1f3ab92e4 > src/arch/sparc/isa/decoder.isa dee1f3ab92e4 > src/arch/x86/isa/microops/fpop.isa dee1f3ab92e4 > src/arch/x86/isa/microops/ldstop.isa dee1f3ab92e4 > src/arch/x86/isa/microops/limmop.isa dee1f3ab92e4 > src/arch/x86/isa/microops/mediaop.isa dee1f3ab92e4 > src/arch/x86/isa/microops/regop.isa dee1f3ab92e4 > > Diff: http://reviews.m5sim.org/r/873/diff > > > Testing > ------- > > > Thanks, > > Gabe > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
