> On May 14, 2015, 6:10 p.m., Joel Hestness wrote: > > src/mem/slicc/symbols/StateMachine.py, line 629 > > <http://reviews.gem5.org/r/2801/diff/1/?file=45050#file45050line629> > > > > I see. On my first pass here, I misread that this code allows arbitrary > > expressions for setting buffer_size. You're right that the parser disallows > > that. > > > > However, you should be using the pair values' types here to be > > consistent with other code generation rather than using isinstance, which > > is very out-of-place here. Each PairAST value has a type. From the parser: > > > > def p_pair__assign(self, p): > > """pair : ident '=' STRING > > | ident '=' ident > > | ident '=' NUMBER""" > > p[0] = ast.PairAST(self, p[1], p[3]) > > > > And for instance, the parser sets STRING types as follows: > > > > def t_STRING1(self, t): > > r'\"[^"\n]*\"' > > t.type = 'STRING' > > t.value = t.value[1:-1] > > return t > > > > Thus, you should be able to get the buffer_size value's type with code > > like: > > > > size_type = var["buffer_size"].type > > > > Then you can predicate code generation on whether the type is IDENT, > > STRING, or INT like other code here (e.g. see 'vtype' local var). > > > > This same issue applies in review request 2814 ( > > http://reviews.gem5.org/r/2814/ ), which looks like it is just replicating > > this code? > > Brad Beckmann wrote: > What you describe sounds like it may work as well, but I don't see why it > is any better than the current implementation. Both provide the same > feature. It doesn't seem worth the effor to reimplement this just because > you feel the use of isinstance is out-of-place. > > Mark Wilkening wrote: > This code seems to follow the conventions used for other properties of > message buffers (network, virtual network id, ...). I do not think we need to > change this unless we want to completely change how message buffers are > handled and make an AST (which I would not reccommend).
It's not just that isinstance is out-of-place, it's actually incorrect: You are currently comparing the types of the Python objects that hold the SLICC AST, but you SHOULD be comparing the types defined in the AST itself. These are not necessarily equivalent, nor should we assume that they are. - Joel ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2801/#review6272 ----------------------------------------------------------- On May 11, 2015, 10:20 p.m., Tony Gutierrez wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2801/ > ----------------------------------------------------------- > > (Updated May 11, 2015, 10:20 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 10858:b1d4246c12ad > --------------------------- > slicc: Support for setting individual message buffer size > > This patch adds support for setting per-MessageBuffer buffer sizes. Prior to > this patch, all buffers used the same size that was set globally. This patch > also adds a "kill switch" that turns all buffers back into infinite > buffers. The configuration will print warnings when either the kill switch is > on and an attempt is made to set a finite buffer size or the kill switch is > off and there is an attempt to set an inifinite buffer size. > > The global buffer size variable still exists, and is now turned into a default > value if no more specific value is set (such as internal buffers created by > switches). > > > Diffs > ----- > > src/mem/ruby/network/MessageBuffer.hh > fbdaa08aaa426b9f4660c366f934ccb670d954ec > src/mem/ruby/network/MessageBuffer.cc > fbdaa08aaa426b9f4660c366f934ccb670d954ec > src/mem/ruby/system/RubySystem.py fbdaa08aaa426b9f4660c366f934ccb670d954ec > src/mem/ruby/system/System.hh fbdaa08aaa426b9f4660c366f934ccb670d954ec > src/mem/ruby/system/System.cc fbdaa08aaa426b9f4660c366f934ccb670d954ec > src/mem/slicc/symbols/StateMachine.py > fbdaa08aaa426b9f4660c366f934ccb670d954ec > > Diff: http://reviews.gem5.org/r/2801/diff/ > > > Testing > ------- > > > Thanks, > > Tony Gutierrez > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
