Review Board 2.0.15


ruby: change MultiBitSelBlockFilter constructor signature

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

Information
Brandon Potter
gem5
default
Reviewers
Default 
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!



   

   
Jason Lowe-Power

   
Posted  (April 5, 2016, 7:59 a.m.)


  


  


  

   



  

   

   
    
    
   


  




   







    

     
     
    

    


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


     





 

  
  
  
  
  
  Show all issues
 




     




  

   

   
    
    

   
  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. 


   
  2. 
    
    
     
    
      
     
    
     
    Not at all.
    
    
    
   
    3. 

   


  


    


   



   
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. 


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