From Peter Lane:

Since you're seeking a code review of the data structure code, I'd like to add my $0.02 based on what I see in your dev branch (hopefully that's the latest version).

1. We need to merge the enums in GlobalReconEnums.hh with those in Detector.hh and Particle.hh. Selfishly I'd like to see the ones I've been using be adopted as much as possible from it's current form to avoid too much recoding, but I'm open to suggestions on changes you see necessary. Nit picking a bit as well, I don't like the feel of having a header for just the enums in DataStructure. It doesn't strike me as a very C++ thing to do.
2. I'm not sure I like the idea of having the classes in the MAUS::recon::global and then typedefing them into the MAUS namespace and tacking on "Global" on the front. I'm inclined to say that they should all just be Global* classes in the MAUS namespace. They aren't really serving as functional components to the internals of the reconstruction. Like all the other classes in DataStructure they are intended for data passing. So in some sense they are polluting the MAUS::recon::global namespace.
3. It looks like GlobalTrackPoint should be a subclass of GlobalSpacePoint.
4. Nit pick: if the class name is SpacePoint then an instance variable should be space_point, not spacepoint. Same for TrackPoint.
5. I like author comments in source code files. It lets me know who to talk to if I need to ask a question about something.
6. I don't like the *FromInt function names in GlobalPrimaryChain. It doesn't tell you what the int you are using represents. Better: *At, *AtIndex, etc...
7. I agree with the comment for CleanUpTracks in GlobalPrimaryChain about never removing tracks. Again, these are data passing classes. You shouldn't be using them internally in algorithms where you might need to move tracks around. Setup the instance once how you want it transmitted and leave it alone. Recommend deleting this function as well as the Add* and Delete* functions. The std::vector class has sufficient capabilities along these lines.

And for amusing attempts to push conformance to the Google coding standards we're supposed to adhere to:

1. Member variables end, not begin, with an underscore. (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Variable_Names)
2. Member variable getters are just the name of the instance variable without the underscore. (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Names)

Peter

From Chris Rogers (actions on Ian unless stated)

Nb: reviewed revision 706 of bzr+ssh://bazaar.launchpad.net/~i-taylor/maus/rc/

1. Worth commenting that TObject inheritance should be confined to DataStructure
2. Can probably use TObject inheritance instead of template <class ChildType>
3. action on ROGERS: Remove ObjectProcessor::RegisterPointerReference - this is superceded by RegisterTRef
4. A comment on TRefArray "gotchas" would be useful - given that you've learnt from experience

1. Remove typedefs - instead use MAUS::DataStructure::Global namespace, and reflect in the directory structure
2. Add a pointer from PrimaryChain going back down the global recon tree of PrimaryChains - to allow more sophisticated back tracing than the mapper_name string
3. Instances of Squeak::mout(Squeak::error) should probably be an exception
4. Careful about memory ownership. If a class owns memory, should always have in mutator something like
   

   if (_data != NULL) {
       delete _data;
   }
   _data = new_data;
   

   
   Functions which take a pointer as an input should always be clear about who owns the memory before/after the function call - and this should be commented in the header doxygen.
5. Add global datastructure to the doc/doc_src/spill_structure.dot
6. Update data_structure.tex subsection about pointers and memory ownership
7. Check test coverage (change the maus_lcov environment variable before running scons -c; scons; test/unit_tests.bash

With a little more searching, this seems to conform to our rules, although it is also a little bit of Black Magic:

http://stackoverflow.com/questions/2633787/compile-time-type-determination-in-c

Any complaints before I implement it?

Just to be clear - style guide is supposed to be a set of reasonable things one can do to make the code better. Don't make the code worse to obey style guide.

Request from Victoria Blackmore @ CM35:

Add a data element to PrimaryChain, which tracks any potential issues or comments for analysers to be aware of in an event.

Would propose a std::map<"std::string", "std::string">, where first is an error/instance code, and second is details of the error/instance: e.g.

"OutsideTracker","Track left Tracker geometry region @ X:120, Y:-100, Z:-400"

1. The enums are not strictly related to data passing, so I'd suggest just typedefing the enums from the recon code if you want a DS branded copy.

2. In that case the OOP thing to do would be to create a third class (GlobalPointBase or something) with common elements and have them both inherit it.

1. Discussed this with Chris Rogers to confirm the MAUS design strategy. DataStructure code shoudl not depend on other elements, and should form the base of MAUS. As the enums are being written into the data, it is important that they be defined inside the data structure (a future situation can be imagined, where data from MAUS 1.4.3 is analysed with MAUS 1.4.5, because we have confirmed that the DataStructure code did not change between the two versions, but the analysis tools did; this only works if DataStructure is self-contained). However, it should be trivial for me to import the current enums, add any elements that are missing and then put a typedef into the current GlobalRecon enum locations.
2. We could, but we don't have to, and I don't see a compelling argument to do so.

1. Point taken.
2. No, of course you don't have to. But one of the main ideas of using an OO language is to take advantage of code reuse to avoid inconsistencies resulting from having to keep multiple pieces of code in sync. If the similar code is similar because it acts on the same subset of data then there ought to be a common definition in the data structure for that subset. Perhaps we've done our jobs perfectly and the DS subset will never change, but who knows. The whole point is that you don't have to worry about it because you've guaranteed that they can't go out of sync by design. TrackPoint and SpacePoint obviously qualify as two objects that contain the same subset of data. Anyway, that's my attempt at a compelling argument. Take it or leave it.

1. And... you can't typedef enums that way. How very annoying! Going for the minimally painful option of changing Peter's code to point to DataStructure::Global for his enums...

2. Okay, I accept the argument.

1. Could you elaborate. I wrote a simple test program that typedefed an enum as a class member and it worked fine.

Sorry, was rushing out the door to the field mitigation meeting, shouldn't have written such a short comment.

You can indeed typedef the enum, i.e. the following works and compiles:

typedef MAUS::DataStructure::Global::DetectorPoint ID;

However, this only typedefs the enum, it does not provide the values. This means that you get a failure from: Detector::kUndefined, with an error message claiming (rightly) that kUndefined is not a member of MAUS::recon::global::Detector.

I can get around this by adding:

using namespace MAUS::DataStructure::Global;

to every file and changing Detector::kUndefined to kUndefined, but seeing as that is a stupid thing to do, I'm not going to.

I'm about to call it a day, so if you have a snazy solution, then I'll implement it in the morning. Otherwise, I'll work through the code and make the necessary changes.

Ah, enum doesn't create it's own scope. Rats! Ok, it appears that I've lost this battle. Do what you need to do.

Is this now closed?