Review Board 2.0.15

ruby: change MultiBitSelBlockFilter constructor signature

Review Request #3420 - Created April 4, 2016 and updated

Brandon Potter
Changeset 11422:6fc675f956dc
ruby: change MultiBitSelBlockFilter constructor signature

The previous format required a string which was then tokenized to retrieve
the constructor arguments. The string is removed and the arguments are
passed directly in their correct format.


Issue Summary

1 0 0 1
Description From Last Updated Status
Ship it!
Posted (April 5, 2016, 7:59 a.m.)


Why this change? isParallel is what the style guide says, not m_*.

  1. The rest of the class uses the m_* notation so it's an issue of ruby conventions clashing with gem5 conventions. Moreover, slicc does the conversion where it adds the m_ automatically when it generates the controllers and other classes so it's a substantial change to deal with the problem properly.

    gem5 style guide: "Class member names (method and variables, including const variables) are mixed case, start with a lowercase letter, and do not contain underscores (e.g., aMemberVariable). Class members that have accessor methods should have a leading underscore to indicate that the user should be using an accessor. The accessor functions themselves should have the same name as the variable without the leading underscore."

    When I run into conflicts like this, I usually defer to whatever the file contains so the file is at least consistent.

    Do you mind if I drop the issue?

  2. Not at all.

Couple of small notes, but other than that it's fine.

Any idea why it was originally like this? Seems kinda crazy.

  1. I do not know anything about this code or how it was suppossed to be used. I inherited this alteration from Nilay in a larger patch and factored it out here for posting.

    From what I can tell, nothing includes the header for this filter class. The src directory only shows that the cc file needs it. After running the util/regress script, I search for the include with
    find ./build -type f | xargs grep -I "#include \".*MultiBitSelBloomFilter.hh\""; I still come up with nothing.

    The change was made in the context of these other patches because code around this alteration gets touched.

  2. Yeah, the change seems fine. I was just curious.