Fix compilation warnings

Compilation warnings may indicate unwanted behavior, point to sources of inefficiency, and mask more serious problems. Efforts should be made to eliminate them.

Collecting notes on a campaign to reduce the volume of compilation warnings.

Arguments shadowing data members

A number of the warnings have the form

warning: declaration of ‘<variable name>’ shadows a member of 'this'

These often occur in class constructors, where the value of the argument that shadows the data member is used to initialize the data member. In these cases, the end result is often what is desired, despite the warning. However, it is easy to create instances that have undesired effects, even in constructors, especially if the argument is passed by reference. This kind of pattern should really be avoided.

Many large toolkits avoid such problems by adhering to naming conventions for data members and arguments. For example, GEANT4 appears to conform to a convention in which every class private data member has a name that begins with f. For classes in which i encounter a shadowing warning for a private data member, i will change the names of the private data members to begin with f, as GEANT4 does.

In some cases, an argument name shadows a public data member. For these cases, i will chang the arguments' names, either by abbreviating them or expanding them.

Warnings in WCSimWCPMT

Warning text

Compiling WCSimWCPMT.cc ...
src/WCSimWCPMT.cc: In constructor ‘WCSimWCPMT::WCSimWCPMT(G4String, WCSimDetectorConstruction*, G4String)’:
src/WCSimWCPMT.cc:33:48: warning: declaration of ‘detectorElement’ shadows a member of 'this' [-Wshadow]
                        G4String detectorElement)
                                                ^
src/WCSimWCPMT.cc:33:48: warning: declaration of ‘myDetector’ shadows a member of 'this' [-Wshadow]
src/WCSimWCPMT.cc: In member function ‘void WCSimWCPMT::MakePeCorrection(WCSimWCHitsCollection*)’:
src/WCSimWCPMT.cc:179:16: warning: declaration of ‘peSmeared’ shadows a member of 'this' [-Wshadow]
       G4double peSmeared = 0.0;
                ^
src/WCSimWCPMT.cc:148:20: warning: unused variable ‘PMT’ [-Wunused-variable]
   WCSimPMTObject * PMT = myDetector->GetPMTPointer(WCCollectionName);
                    ^

Shadowing

The data members shadowed by the constructor arguments myDetector and detectorElement are public data members. I abbreviated the argument names to myDet and detElem respectively.

I also noticed that the data member myDetector is initialized in the body of the constructor, but it could be added to the initializer list. So i updated the constructor to do so.

The variable peSmeared defined in WCSimWCPMT::MakePeCorrection() also shadows a public data member. In this case, i can find no instance in which the data member WCSimWCPMT::peSmeared is ever used. Not in the function members of WCSimWCPMT nor in any function that uses WCSimWCPMT. I am not sure what purpose WCSimWCPMT::peSmeared was meant to serve. Maybe as a named cache for a value? I removed the data member, as it does not appear to be used and has no discernable purpose.

Unused variable

Yes, the variable PMT is never used in the body of WCSimWCPMT::MakePeCorrection(), not even in knocked-out code. It is initialized with a call to the function WCSimDetectorConstruction::GetPMTPointer(), but this function does not perform any operations that modify any data members---it's an inline accessor method. Removed the unused variable PMT.

Testing with optional flag `HYPER_VERBOSITY`

Compiling with #define HYPER_VERBOSITY uncommented produces no additional compiler warnings.

Conclusion

After these changes, there are no more compiler warnings from WCSimWCPMT.

Warnings in WCSimWCDigitizer

Warning text

Compiling WCSimWCDigitizer.cc ...
src/WCSimWCDigitizer.cc: In constructor ‘WCSimWCDigitizerBase::WCSimWCDigitizerBase(G4String, WCSimDetectorConstruction*, WCSimWCDAQMessenger*, DigitizerType_t, G4String)’:
src/WCSimWCDigitizer.cc:26:38: warning: declaration of ‘detectorElement’ shadows a member of 'this' [-Wshadow]
              G4String detectorElement)
                                      ^
In file included from src/WCSimWCDigitizer.cc:1:0:
include/WCSimWCDigitizer.hh:70:19: warning: ‘WCSimWCDigitizerBase::DigitizerType’ will be initialized after [-Wreorder]
   DigitizerType_t DigitizerType; ///< Enumeration to say which digitizer we've constructed
                   ^
include/WCSimWCDigitizer.hh:64:12: warning:   ‘G4String WCSimWCDigitizerBase::DigitizerClassName’ [-Wreorder]
   G4String DigitizerClassName;    ///< Name of the digitizer class being run
            ^
src/WCSimWCDigitizer.cc:22:1: warning:   when initialized here [-Wreorder]
 WCSimWCDigitizerBase::WCSimWCDigitizerBase(G4String name,
 ^
src/WCSimWCDigitizer.cc: In member function ‘bool WCSimWCDigitizerBase::AddNewDigit(int, int, double, double, std::vector<int>)’:
src/WCSimWCDigitizer.cc:116:124: warning: declaration of ‘peSmeared’ shadows a member of 'this' [-Wshadow]
 bool WCSimWCDigitizerBase::AddNewDigit(int tube, int gate, double digihittime, double peSmeared, std::vector<int> digi_comp)

                                            ^
src/WCSimWCDigitizer.cc: In constructor ‘WCSimWCDigitizerSKI::WCSimWCDigitizerSKI(G4String, WCSimDetectorConstruction*, WCSimWCDAQMessenger*, G4String)’:
src/WCSimWCDigitizer.cc:191:66: warning: declaration of ‘detectorElement’ shadows a member of 'this' [-Wshadow]
                                          G4String detectorElement)
                                                                  ^
src/WCSimWCDigitizer.cc:191:66: warning: declaration of ‘myDetector’ shadows a member of 'this' [-Wshadow]

Shadowing

All of the data members of WCSimWCDigitizerBase are protected, so i will use the same solution as that for private data members. To be consistent, i changed the names of all of the data members of WCSimWCDigitizerBase to begin with f.

Initializer order

The constructor's initializer list should be in the same order as the declaration of the data members in the header file. Simple rearrangement of the initializers.

Testing with the optional compilation flags

`WCSIMWCDIGITIZER_VERBOSE`

Uncommenting #define WCSIMWCDIGITIZER_VERBOSE does not uncover new compiler warnings.

`HYPER_VERBOSITY`

Uncommenting #define HYPER_VERBOSITY does not uncover new compiler warnings.

Conclusion

After these changes, there are no more compiler warnings from WCSimWCDigitizer.

Warnings in WCSimTrajectory

Warning text

Compiling WCSimTrajectory.cc ...
In file included from include/WCSimTrajectory.hh:6:0,
                 from src/WCSimTrajectory.cc:1:
/data/neutrino06/pspradlin/hk-hyperk/Geant4/install/include/Geant4/G4VTrajectory.hh:91:17: warning: ‘virtual void G4VTrajectory::DrawTrajectory() const’ was hidden [-Woverloaded-virtual]
    virtual void DrawTrajectory() const;
                 ^
In file included from src/WCSimTrajectory.cc:1:0:
include/WCSimTrajectory.hh:78:17: warning:   by ‘virtual void WCSimTrajectory::DrawTrajectory(G4int) const’ [-Woverloaded-virtual]
    virtual void DrawTrajectory(G4int i_mode=0) const;
                 ^
src/WCSimTrajectory.cc:96:6: warning: unused parameter ‘i_mode’ [-Wunused-parameter]
 void WCSimTrajectory::DrawTrajectory(G4int i_mode) const
      ^
src/WCSimTrajectory.cc: In member function ‘virtual std::vector<G4AttValue>* WCSimTrajectory::CreateAttValues() const’:
src/WCSimTrajectory.cc:140:25: warning: declaration of ‘s’ shadows a global declaration [-Wshadow]
   std::ostringstream s(c);
                         ^
In file included from /data/neutrino06/pspradlin/hk-hyperk/Geant4/install/include/Geant4/CLHEP/Units/PhysicalConstants.h:42:0,
                 from /data/neutrino06/pspradlin/hk-hyperk/Geant4/install/include/Geant4/G4ParticleDefinition.hh:58,
                 from include/WCSimTrajectory.hh:12,
                 from src/WCSimTrajectory.cc:1:
/data/neutrino06/pspradlin/hk-hyperk/Geant4/install/include/Geant4/CLHEP/Units/SystemOfUnits.h:141:24: warning: shadowed declaration is here [-Wshadow]
   static const double  s = second;
                        ^

There are some interesting things going on here.

Shadowing `s`

The CLHEP SystemOfUnits.h puts a lot variables in the global namespace that represent common units. Remember to avoid variable names that defined names for units. Here, the local variable that shadows the global unit s is a stringstream text buffer. Rename it to st. I like the compactness of the one-letter variable name, but i do not see a nice one-letter substitute that makes the distinction between string and char as well as the current implementation's s and c do.

WCSimTrajectory::DrawTrajectory()

This is interesting. The WCSimTrajectory::DrawTrajectory() method overloads a method that is defined in the base class G4VTrajectory. The derived instance looks like it is a placeholder for something that hasn't been implemented. At the moment, it just invokes the base class method:

void WCSimTrajectory::DrawTrajectory(G4int i_mode) const
{
  // Invoke the default implementation in G4VTrajectory...
  G4VTrajectory::DrawTrajectory();
  // ... or override with your own code here.
}

Although the argument for the derived class method does nothing, the only invocation DrawTrajectory() passes an argument to it (in WCSimEventAction::EndOfEventAction()). Further, although the name of the argument, i_mode, suggests some kind of modal behavior, the value 50 passed into it is not obviously a modal index used anywhere else in WCSimEventAction::EndOfEventAction().

I cannot make an easy fix to this. This probably deserves its own issue to discuss what it is doing and what it is supposed to be doing.

Inconclusive

I have created an issue stub to follow-up on this.

Warnings in WCSimWCSD

Warning text

Compiling WCSimWCSD.cc ...
src/WCSimWCSD.cc: In constructor ‘WCSimWCSD::WCSimWCSD(G4String, G4String, WCSimDetectorConstruction*, G4String)’:
src/WCSimWCSD.cc:23:46: warning: declaration of ‘detectorElement’ shadows a member of 'this' [-Wshadow]
                      G4String detectorElement)
                                              ^

Straightforward argument-shadowing-data-member name. Updated the names of the private data members to begin with f.

Warnings in WCSimWCAddDarkNoise

Warning text

Compiling WCSimWCAddDarkNoise.cc ...
src/WCSimWCAddDarkNoise.cc: In constructor ‘WCSimWCAddDarkNoise::WCSimWCAddDarkNoise(G4String, WCSimDetectorConstruction*, G4String)’:
src/WCSimWCAddDarkNoise.cc:39:66: warning: declaration of ‘detectorElement’ shadows a member of 'this' [-Wshadow]
                                          G4String detectorElement)
                                                                  ^
src/WCSimWCAddDarkNoise.cc: In member function ‘void WCSimWCAddDarkNoise::AddDarkNoise()’:
src/WCSimWCAddDarkNoise.cc:156:9: warning: unused variable ‘windowfordarknoise’ [-Wunused-variable]
     int windowfordarknoise=0;
         ^

Shadowing

A private data member is shadowed by a constructor argument name. Updated the data member names to begin with f.

Unused variable

The variable windowfordarknoise in WCSimWCAddDarkNoise::AddDarkNoise()’ *is* used, but only if compiler macroHYPER_VERBOSITYis set. There are several blocks of optionally compiled code in#ifdef HYPER_VERBOSITYblocks. The variablewindowfordarknoiseis defined outside of these blocks, but only used within them. I moved the variable definition into an appropriateHYPER_VERBOSITY` block.

Testing with the optional compilation flags

`WCSIMWCADDDARKNOISE_VERBOSE`

Uncommenting #define WCSIMWCADDDARKNOISE_VERBOSE does not uncover new compiler warnings.

`HYPER_VERBOSITY`

Uncommenting #define HYPER_VERBOSITY does not uncover new compiler warnings.

`NPMTS_VERBOSE`

Commenting out #define NPMTS_VERBOSE causes compilation to fail. This is not an optional macro.

Conclusion

After these changes, there are no more compiler warnings from WCSimWCAddDarkNoise.

Warnings in WCSimPrimaryGeneratorAction

Warning text

Compiling WCSimPrimaryGeneratorAction.cc ...
src/WCSimPrimaryGeneratorAction.cc: In constructor ‘WCSimPrimaryGeneratorAction::WCSimPrimaryGeneratorAction(WCSimDetectorConstruction*)’:
src/WCSimPrimaryGeneratorAction.cc:122:12: warning: variable ‘cosThetaMean’ set but not used [-Wunused-but-set-variable]
     double cosThetaMean, cosThetaMin, cosThetaMax;
            ^
src/WCSimPrimaryGeneratorAction.cc:122:26: warning: variable ‘cosThetaMin’ set but not used [-Wunused-but-set-variable]
     double cosThetaMean, cosThetaMin, cosThetaMax;
                          ^
src/WCSimPrimaryGeneratorAction.cc:122:39: warning: variable ‘cosThetaMax’ set but not used [-Wunused-but-set-variable]
     double cosThetaMean, cosThetaMin, cosThetaMax;
                                       ^
src/WCSimPrimaryGeneratorAction.cc:123:12: warning: variable ‘phiMean’ set but not used [-Wunused-but-set-variable]
     double phiMean, phiMin, phiMax;
            ^
src/WCSimPrimaryGeneratorAction.cc:123:21: warning: variable ‘phiMin’ set but not used [-Wunused-but-set-variable]
     double phiMean, phiMin, phiMax;
                     ^
src/WCSimPrimaryGeneratorAction.cc:123:29: warning: variable ‘phiMax’ set but not used [-Wunused-but-set-variable]
     double phiMean, phiMin, phiMax;
                             ^

These warnings may indicate a real issue. These unused variables are part of the parsing input file to populate the histograms for simulating the cosmic muon flux. In the input files, like data/MuonFlux-HyperK-ThetaPhi.dat, the values that are parsed into these unused variables appear to define the bin centers and boundaries. The cosimic histos are two-dimensional in polar angles theta and phi.

This looks like an incomplete implementation to me. Instead of using the parsed values to establish the bin boundaries of the generator histograms, the WCSimPrimaryGeneratorAction constructor uses its own hard-coded bin partition without even checking for consistency. This is an issue waiting to happen.

Inconclusive

I have created an issue stub to follow-up on this.

Warnings in WCSimRootGeom

Warning text

Compiling WCSimRootGeom.cc ...
src/WCSimRootGeom.cc: In copy constructor ‘WCSimRootPMT::WCSimRootPMT(const WCSimRootPMT&)’:
src/WCSimRootGeom.cc:79:1: warning: base class ‘class TObject’ should be explicitly initialized in the copy constructor [-Wextra]
 WCSimRootPMT::WCSimRootPMT(const WCSimRootPMT & in)
 ^
src/WCSimRootGeom.cc: In member function ‘void WCSimRootGeom::SetPMT(Int_t, Int_t, Int_t, Double_t*, Double_t*, bool)’:
src/WCSimRootGeom.cc:111:19: warning: unused variable ‘jPMT’ [-Wunused-variable]
     WCSimRootPMT *jPMT = new(pmtArray[i]) WCSimRootPMT(tubeno, cyl_loc, rot, pos);
                   ^

Base class initialization

The base class initialization throughout src/WCSimRootGeom.cc is inconsistent. The source file includes four constructors, a default constructor and a copy constructor for each of WCSimRootGeom and WCSimRootPMT. Both of the classes inherit from Root's TObject class. For WCSimRootGeom,

The default constructor does not explicitly invoke any base class constructor; it implicitly invokes the default constructor for the base class.
The copy constructor explicitly invokes the default constructor for the base class.

The last point seems odd. Usually, to be consistent, one would invoke the copy constructor for the base class in a copy constructor for a derived class.

I added an explicit invocation of the base class copy constructor to the copy constructors of both WCSimRootGeom and WCSimRootPMT.

WCSimRootGeom::SetPMT

The function WCSimRootGeom::SetPMT() is short and appears to be roughly an insert method for the owned container of WCSimRootPMT objects. It creates a new WCSimRootPMT at an indexed location in the owned container. The new WCSimRootPMT object is initialized with arguments to the SetPMT() method.

The insertion of the new WCSimRootPMT object on the rhs of the assignment is necessary. However, we do not need a new variable to hold the pointer returned by it. I deleted the definition of jPMT and left just the call to new. The result now has the same form as a corrsponding call in the WCSimRootGeom copy constructor.

Although i haven't made it so, note that WCSimRootGeom::SetPMT() is simple enough to be inlined.

Conclusion

After these changes, there are no more compiler warnings from WCSimRootGeom.

Warnings in WCSimConstructPMT.cc

src/WCSimConstructPMT.cc is an implementation file for the WCSimDetectorConstruction class.

Warning text

Compiling WCSimConstructPMT.cc ...
src/WCSimConstructPMT.cc: In member function ‘G4LogicalVolume* WCSimDetectorConstruction::ConstructPMT(G4String, G4String, G4String, bool)’:
src/WCSimConstructPMT.cc:232:21: warning: unused variable ‘physiLightCone’ [-Wunused-variable]
  G4VPhysicalVolume* physiLightCone =
                     ^
src/WCSimConstructPMT.cc: In member function ‘G4LogicalVolume* WCSimDetectorConstruction::ConstructPMTAndWLSPlate(G4String, G4String, G4String)’:
src/WCSimConstructPMT.cc:369:44: warning: left operand of comma operator has no effect [-Wunused-value]
       = new G4VisAttributes(G4Colour((0.0, 1.0, 0.0)));
                                            ^
src/WCSimConstructPMT.cc:369:49: warning: right operand of comma operator has no effect [-Wunused-value]
       = new G4VisAttributes(G4Colour((0.0, 1.0, 0.0)));
                                                 ^
src/WCSimConstructPMT.cc:475:24: warning: unused variable ‘physiWLSCladding’ [-Wunused-variable]
     G4VPhysicalVolume* physiWLSCladding =
                        ^
src/WCSimConstructPMT.cc:463:22: warning: unused variable ‘physiWLS’ [-Wunused-variable]
   G4VPhysicalVolume* physiWLS =
                      ^
src/WCSimConstructPMT.cc:488:22: warning: unused variable ‘physiPMT’ [-Wunused-variable]
   G4VPhysicalVolume* physiPMT =
                      ^

Comma splice

The warnings about the comma operators in a G4Colour constructor almost certainly indicate a bug:

  G4VisAttributes* visContainer
      = new G4VisAttributes(G4Colour((0.0, 1.0, 0.0)));

The intent appears to be to specify a color via RGB intensities. What happens instead is that the inner set of parentheses (0.0, 1.0, 0.0) is evaluated as set of comma-spliced expressions that, overall, evaluates to the last value, 0.0. This value is then passed to a G4Colour constructor. Interestingly, the constructor for G4Colour that is invoked is the RGB constructor with the G and B values set to defaults (1.0), so that the whole expression is equivalent to G4VisAttributes(G4Colour(0.0, 1.0, 1.0));.

Anyway, easy bug fix: just removed the inner set of parentheses. This should change the display color for the relevant component. It can be restored by coding in the correct RGB values.

Unused variables

These are all variables for pointers returned by allocating new volume elements on the free store. These variables are never used. The allocation is necessary, but the pointers are never used directly. I do not like to leave unused code in comments, but in this case i decided to just comment out the the variable declarations and assignments that lead to these warnings.

Conclusion

After these changes, there are no more compiler warnings from WCSimConstructPMT.cc.

Some follow-up may be indicated on the choice to comment out the declarations of the unused variable.

Warnings in WCSimConstructMaterials.cc

src/WCSimConstructMaterials.cc is an implementation file for class WCSimDetectorConstruction.

Warning text

Compiling WCSimConstructMaterials.cc ...
src/WCSimConstructMaterials.cc: In member function ‘void WCSimDetectorConstruction::ConstructMaterials()’:
src/WCSimConstructMaterials.cc:30:15: warning: unused variable ‘Vacuum’ [-Wunused-variable]
   G4Material* Vacuum =
               ^
src/WCSimConstructMaterials.cc:301:12: warning: unused variable ‘PPCKOV’ [-Wunused-variable]
   G4double PPCKOV[NUMENTRIES] =
            ^
src/WCSimConstructMaterials.cc:519:13: warning: unused variable ‘MIE_water’ [-Wunused-variable]
    G4double MIE_water[NUMENTRIES_water] = {
             ^
src/WCSimConstructMaterials.cc:551:13: warning: unused variable ‘MIE_water_const’ [-Wunused-variable]
    G4double MIE_water_const[3]={0.4,0.,1};// gforward, gbackward, forward backward ratio
             ^
src/WCSimConstructMaterials.cc:717:13: warning: unused variable ‘MIE_air’ [-Wunused-variable]
    G4double MIE_air[NUMENTRIES_water] =
             ^
src/WCSimConstructMaterials.cc:729:13: warning: unused variable ‘MIE_air_const’ [-Wunused-variable]
    G4double MIE_air_const[3]={0.99,0.99,0.8};// gforward, gbackward, forward backward ratio
             ^
src/WCSimConstructMaterials.cc:893:10: warning: unused variable ‘no_absorption’ [-Wunused-variable]
   double no_absorption = 1000.*m;
          ^
src/WCSimConstructMaterials.cc:894:10: warning: unused variable ‘immediate_absorption’ [-Wunused-variable]
   double immediate_absorption = 0.*m;
          ^

Unused variable `Vacuum`

This is a case in which a necessary allocator returns a pointer that is never used directly:

  G4Material* Vacuum = 
    new G4Material("Vacuum", 1., a, density,
                   kStateGas,temperature,pressure);

I commented out the variable declaration and assignment, but left the allocation call.

Unused property values

src/WCSimConstructMaterials.cc is riddled with what appear to be commented-out data tables. I do not know much about their history, but i would guess that this mess evolved organically, and that some of the tables are commented out rather than removed for ease of selecting alternatives. This is not a good way to handle this kind of thing.

For now, i can comment-out the unused arrays. This looks like it should have some follow-up and review to clean up the vestigial code.

Conclusion

After these changes, there are no more compiler warnings from WCSimConstructMaterials.cc.

However, the vestigial code in it should be reviewed and cleaned up.

Warnings in WCSimSteppingAction

Warning text

Compiling WCSimSteppingAction.cc ...
In file included from /data/neutrino06/pspradlin/hk-hyperk/root_build/include/TClass.h:26:0,
                 from /data/neutrino06/pspradlin/hk-hyperk/root_build/include/TBuffer.h:26,
                 from /data/neutrino06/pspradlin/hk-hyperk/root_build/include/TDirectory.h:24,
                 from /data/neutrino06/pspradlin/hk-hyperk/root_build/include/TDirectoryFile.h:25,
                 from /data/neutrino06/pspradlin/hk-hyperk/root_build/include/TFile.h:27,
                 from include/WCSimRunAction.hh:8,
                 from include/WCSimSteppingAction.hh:9,
                 from src/WCSimSteppingAction.cc:7:
/data/neutrino06/pspradlin/hk-hyperk/root_build/include/TObjString.h: In constructor ‘TObjString::TObjString(const char*)’:
/data/neutrino06/pspradlin/hk-hyperk/root_build/include/TObjString.h:34:35: warning: declaration of ‘s’ shadows a global declaration [-Wshadow]
    TObjString(const char *s = "") : fString(s) { }
                                   ^
In file included from src/WCSimSteppingAction.cc:4:0:
/data/neutrino06/pspradlin/hk-hyperk/Geant4/install/include/Geant4/G4SIunits.hh:150:22: warning: shadowed declaration is here [-Wshadow]
 static const double  s = second;
                      ^
In file included from /data/neutrino06/pspradlin/hk-hyperk/root_build/include/TClass.h:26:0,
                 from /data/neutrino06/pspradlin/hk-hyperk/root_build/include/TBuffer.h:26,
                 from /data/neutrino06/pspradlin/hk-hyperk/root_build/include/TDirectory.h:24,
                 from /data/neutrino06/pspradlin/hk-hyperk/root_build/include/TDirectoryFile.h:25,
                 from /data/neutrino06/pspradlin/hk-hyperk/root_build/include/TFile.h:27,
                 from include/WCSimRunAction.hh:8,
                 from include/WCSimSteppingAction.hh:9,
                 from src/WCSimSteppingAction.cc:7:
/data/neutrino06/pspradlin/hk-hyperk/root_build/include/TObjString.h: In member function ‘void TObjString::SetString(const char*)’:
/data/neutrino06/pspradlin/hk-hyperk/root_build/include/TObjString.h:45:41: warning: declaration of ‘s’ shadows a global declaration [-Wshadow]
    void        SetString(const char *s) { fString = s; }
                                         ^
In file included from src/WCSimSteppingAction.cc:4:0:
/data/neutrino06/pspradlin/hk-hyperk/Geant4/install/include/Geant4/G4SIunits.hh:150:22: warning: shadowed declaration is here [-Wshadow]
 static const double  s = second;
                      ^
In file included from /data/neutrino06/pspradlin/hk-hyperk/root_build/include/TDirectory.h:24:0,
                 from /data/neutrino06/pspradlin/hk-hyperk/root_build/include/TDirectoryFile.h:25,
                 from /data/neutrino06/pspradlin/hk-hyperk/root_build/include/TFile.h:27,
                 from include/WCSimRunAction.hh:8,
                 from include/WCSimSteppingAction.hh:9,
                 from src/WCSimSteppingAction.cc:7:
/data/neutrino06/pspradlin/hk-hyperk/root_build/include/TBuffer.h: In member function ‘virtual void TBuffer::ReadStdString(std::string&)’:
/data/neutrino06/pspradlin/hk-hyperk/root_build/include/TBuffer.h:291:54: warning: declaration of ‘s’ shadows a global declaration [-Wshadow]
    virtual inline void ReadStdString(std::string &s) { ReadStdString(&s); }
                                                      ^
In file included from src/WCSimSteppingAction.cc:4:0:
/data/neutrino06/pspradlin/hk-hyperk/Geant4/install/include/Geant4/G4SIunits.hh:150:22: warning: shadowed declaration is here [-Wshadow]
 static const double  s = second;
                      ^
In file included from /data/neutrino06/pspradlin/hk-hyperk/root_build/include/TDirectory.h:24:0,
                 from /data/neutrino06/pspradlin/hk-hyperk/root_build/include/TDirectoryFile.h:25,
                 from /data/neutrino06/pspradlin/hk-hyperk/root_build/include/TFile.h:27,
                 from include/WCSimRunAction.hh:8,
                 from include/WCSimSteppingAction.hh:9,
                 from src/WCSimSteppingAction.cc:7:
/data/neutrino06/pspradlin/hk-hyperk/root_build/include/TBuffer.h: In member function ‘virtual void TBuffer::WriteStdString(std::string&)’:
/data/neutrino06/pspradlin/hk-hyperk/root_build/include/TBuffer.h:311:55: warning: declaration of ‘s’ shadows a global declaration [-Wshadow]
    virtual inline void WriteStdString(std::string &s) { WriteStdString(&s); }
                                                       ^
In file included from src/WCSimSteppingAction.cc:4:0:
/data/neutrino06/pspradlin/hk-hyperk/Geant4/install/include/Geant4/G4SIunits.hh:150:22: warning: shadowed declaration is here [-Wshadow]
 static const double  s = second;
                      ^
In file included from /data/neutrino06/pspradlin/hk-hyperk/root_build/include/TDirectory.h:24:0,
                 from /data/neutrino06/pspradlin/hk-hyperk/root_build/include/TDirectoryFile.h:25,
                 from /data/neutrino06/pspradlin/hk-hyperk/root_build/include/TFile.h:27,
                 from include/WCSimRunAction.hh:8,
                 from include/WCSimSteppingAction.hh:9,
                 from src/WCSimSteppingAction.cc:7:
/data/neutrino06/pspradlin/hk-hyperk/root_build/include/TBuffer.h: In function ‘TBuffer& operator>>(TBuffer&, Short_t&)’:
/data/neutrino06/pspradlin/hk-hyperk/root_build/include/TBuffer.h:347:52: warning: declaration of ‘s’ shadows a global declaration [-Wshadow]
 inline TBuffer &operator>>(TBuffer &buf, Short_t &s)  { buf.ReadShort(s);  return buf; }
                                                    ^
In file included from src/WCSimSteppingAction.cc:4:0:
/data/neutrino06/pspradlin/hk-hyperk/Geant4/install/include/Geant4/G4SIunits.hh:150:22: warning: shadowed declaration is here [-Wshadow]
 static const double  s = second;
                      ^
In file included from /data/neutrino06/pspradlin/hk-hyperk/root_build/include/TDirectory.h:24:0,
                 from /data/neutrino06/pspradlin/hk-hyperk/root_build/include/TDirectoryFile.h:25,
                 from /data/neutrino06/pspradlin/hk-hyperk/root_build/include/TFile.h:27,
                 from include/WCSimRunAction.hh:8,
                 from include/WCSimSteppingAction.hh:9,
                 from src/WCSimSteppingAction.cc:7:
/data/neutrino06/pspradlin/hk-hyperk/root_build/include/TBuffer.h: In function ‘TBuffer& operator>>(TBuffer&, UShort_t&)’:
/data/neutrino06/pspradlin/hk-hyperk/root_build/include/TBuffer.h:348:53: warning: declaration of ‘s’ shadows a global declaration [-Wshadow]
 inline TBuffer &operator>>(TBuffer &buf, UShort_t &s) { buf.ReadUShort(s); return buf; }
                                                     ^
In file included from src/WCSimSteppingAction.cc:4:0:
/data/neutrino06/pspradlin/hk-hyperk/Geant4/install/include/Geant4/G4SIunits.hh:150:22: warning: shadowed declaration is here [-Wshadow]
 static const double  s = second;
                      ^
In file included from /data/neutrino06/pspradlin/hk-hyperk/root_build/include/TDirectory.h:24:0,
                 from /data/neutrino06/pspradlin/hk-hyperk/root_build/include/TDirectoryFile.h:25,
                 from /data/neutrino06/pspradlin/hk-hyperk/root_build/include/TFile.h:27,
                 from include/WCSimRunAction.hh:8,
                 from include/WCSimSteppingAction.hh:9,
                 from src/WCSimSteppingAction.cc:7:
/data/neutrino06/pspradlin/hk-hyperk/root_build/include/TBuffer.h: In function ‘TBuffer& operator>>(TBuffer&, TString&)’:
/data/neutrino06/pspradlin/hk-hyperk/root_build/include/TBuffer.h:358:52: warning: declaration of ‘s’ shadows a global declaration [-Wshadow]
 inline TBuffer &operator>>(TBuffer &buf, TString &s)  { buf.ReadTString(s);return buf; }
                                                    ^
In file included from src/WCSimSteppingAction.cc:4:0:
/data/neutrino06/pspradlin/hk-hyperk/Geant4/install/include/Geant4/G4SIunits.hh:150:22: warning: shadowed declaration is here [-Wshadow]
 static const double  s = second;
                      ^
In file included from /data/neutrino06/pspradlin/hk-hyperk/root_build/include/TDirectory.h:24:0,
                 from /data/neutrino06/pspradlin/hk-hyperk/root_build/include/TDirectoryFile.h:25,
                 from /data/neutrino06/pspradlin/hk-hyperk/root_build/include/TFile.h:27,
                 from include/WCSimRunAction.hh:8,
                 from include/WCSimSteppingAction.hh:9,
                 from src/WCSimSteppingAction.cc:7:
/data/neutrino06/pspradlin/hk-hyperk/root_build/include/TBuffer.h: In function ‘TBuffer& operator<<(TBuffer&, Short_t)’:
/data/neutrino06/pspradlin/hk-hyperk/root_build/include/TBuffer.h:363:51: warning: declaration of ‘s’ shadows a global declaration [-Wshadow]
 inline TBuffer &operator<<(TBuffer &buf, Short_t s)  { buf.WriteShort(s);  return buf; }
                                                   ^
In file included from src/WCSimSteppingAction.cc:4:0:
/data/neutrino06/pspradlin/hk-hyperk/Geant4/install/include/Geant4/G4SIunits.hh:150:22: warning: shadowed declaration is here [-Wshadow]
 static const double  s = second;
                      ^
In file included from /data/neutrino06/pspradlin/hk-hyperk/root_build/include/TDirectory.h:24:0,
                 from /data/neutrino06/pspradlin/hk-hyperk/root_build/include/TDirectoryFile.h:25,
                 from /data/neutrino06/pspradlin/hk-hyperk/root_build/include/TFile.h:27,
                 from include/WCSimRunAction.hh:8,
                 from include/WCSimSteppingAction.hh:9,
                 from src/WCSimSteppingAction.cc:7:
/data/neutrino06/pspradlin/hk-hyperk/root_build/include/TBuffer.h: In function ‘TBuffer& operator<<(TBuffer&, UShort_t)’:
/data/neutrino06/pspradlin/hk-hyperk/root_build/include/TBuffer.h:364:52: warning: declaration of ‘s’ shadows a global declaration [-Wshadow]
 inline TBuffer &operator<<(TBuffer &buf, UShort_t s) { buf.WriteUShort(s); return buf; }
                                                    ^
In file included from src/WCSimSteppingAction.cc:4:0:
/data/neutrino06/pspradlin/hk-hyperk/Geant4/install/include/Geant4/G4SIunits.hh:150:22: warning: shadowed declaration is here [-Wshadow]
 static const double  s = second;
                      ^
In file included from /data/neutrino06/pspradlin/hk-hyperk/root_build/include/TDirectory.h:24:0,
                 from /data/neutrino06/pspradlin/hk-hyperk/root_build/include/TDirectoryFile.h:25,
                 from /data/neutrino06/pspradlin/hk-hyperk/root_build/include/TFile.h:27,
                 from include/WCSimRunAction.hh:8,
                 from include/WCSimSteppingAction.hh:9,
                 from src/WCSimSteppingAction.cc:7:
/data/neutrino06/pspradlin/hk-hyperk/root_build/include/TBuffer.h: In function ‘TBuffer& operator<<(TBuffer&, const TString&)’:
/data/neutrino06/pspradlin/hk-hyperk/root_build/include/TBuffer.h:374:58: warning: declaration of ‘s’ shadows a global declaration [-Wshadow]
 inline TBuffer &operator<<(TBuffer &buf, const TString &s) { buf.WriteTString(s);return buf; }
                                                          ^
In file included from src/WCSimSteppingAction.cc:4:0:
/data/neutrino06/pspradlin/hk-hyperk/Geant4/install/include/Geant4/G4SIunits.hh:150:22: warning: shadowed declaration is here [-Wshadow]
 static const double  s = second;
                      ^
In file included from /data/neutrino06/pspradlin/hk-hyperk/root_build/include/TMap.h:29:0,
                 from /data/neutrino06/pspradlin/hk-hyperk/root_build/include/TFile.h:28,
                 from include/WCSimRunAction.hh:8,
                 from include/WCSimSteppingAction.hh:9,
                 from src/WCSimSteppingAction.cc:7:
/data/neutrino06/pspradlin/hk-hyperk/root_build/include/THashTable.h: In member function ‘Int_t THashTable::GetHashValue(TString&) const’:
/data/neutrino06/pspradlin/hk-hyperk/root_build/include/THashTable.h:47:47: warning: declaration of ‘s’ shadows a global declaration [-Wshadow]
    Int_t       GetHashValue(TString &s) const { return s.Hash() % fSize; }
                                               ^
In file included from src/WCSimSteppingAction.cc:4:0:
/data/neutrino06/pspradlin/hk-hyperk/Geant4/install/include/Geant4/G4SIunits.hh:150:22: warning: shadowed declaration is here [-Wshadow]
 static const double  s = second;
                      ^
In file included from include/WCSimDetectorConstruction.hh:5:0,
                 from include/WCSimRunAction.hh:14,
                 from include/WCSimSteppingAction.hh:9,
                 from src/WCSimSteppingAction.cc:7:
include/WCSimPMTObject.hh: In member function ‘void WCSimBasicPMTObject::SetgQE(TGraph*)’:
include/WCSimPMTObject.hh:322:25: warning: declaration of ‘g’ shadows a global declaration [-Wshadow]
   void SetgQE(TGraph *g){ gQE=g;};
                         ^
In file included from src/WCSimSteppingAction.cc:4:0:
/data/neutrino06/pspradlin/hk-hyperk/Geant4/install/include/Geant4/G4SIunits.hh:162:23: warning: shadowed declaration is here [-Wshadow]
 static const double   g = gram;
                       ^
src/WCSimSteppingAction.cc: In member function ‘virtual void WCSimSteppingAction::UserSteppingAction(const G4Step*)’:
src/WCSimSteppingAction.cc:35:22: warning: unused variable ‘volume’ [-Wunused-variable]
   G4VPhysicalVolume* volume  = track->GetVolume();
                      ^
src/WCSimSteppingAction.cc:38:16: warning: unused variable ‘SDman’ [-Wunused-variable]
   G4SDManager* SDman   = G4SDManager::GetSDMpointer();
                ^
src/WCSimSteppingAction.cc:39:20: warning: unused variable ‘HCE’ [-Wunused-variable]
   G4HCofThisEvent* HCE = evt->GetHCofThisEvent();
                    ^

Unused variables

These all occur in WCSimSteppingAction::UserSteppingAction(), which is a user hook that appears to do nothing. Most of the code in the WCSimSteppingAction implementation file is vestigial commented-out stuff. I commented out the code in WCSimSteppingAction::UserSteppingAction(), making it another vestige. Should perhaps follow-up to review and clean up what's in here.

Conflicts with named units `s` and `g`

Most of the warnings are due to use of local variables named s and g that shadow the global named constants for physical units of second and gram (see previous comment). In these cases, the use of the local variable has nothing to do with the physical unit. Instead, they are one-letter stand-ins for a type, like string or TGraph.

These appear to be occurring because the G4SIunits.hh header file is included before the header files for the ROOT components, which has the result of bringing the unit constants into the global namespace to conflict with the inlines that are defined in the Root headers.

It surprises me that the G4SIunits.hh constants aren't encapsulated in a namespace.

These warnings can be avoided just by rearranging the #include statements, so i did.

Conclusion

After these changes, there are no more compiler warnings from WCSimSteppingAction.

However, the vestigial code in it should be reviewed and cleaned up.

Warnings in WCSimRunAction

Warnings from WCSimRunAction.hh

include/WCSimRunAction.hh: In member function ‘WCSimRootEvent* WCSimRunAction::GetRootEvent(G4String)’:
include/WCSimRunAction.hh:45:3: warning: control reaches end of non-void function [-Wreturn-type]
   }
   ^
include/WCSimRunAction.hh: In member function ‘TBranch* WCSimRunAction::GetBranch(G4String)’:
include/WCSimRunAction.hh:37:3: warning: control reaches end of non-void function [-Wreturn-type]
   }
   ^

Warnings from WCSimRunAction.cc

Compiling WCSimRunAction.cc ...
src/WCSimRunAction.cc: In member function ‘virtual void WCSimRunAction::BeginOfRunAction(const G4Run*)’:
src/WCSimRunAction.cc:95:12: warning: unused variable ‘geoBranch’ [-Wunused-variable]
   TBranch *geoBranch = geoTree->Branch("wcsimrootgeom", "WCSimRootGeom", &wcsimrootgeom, bufsize,0);
            ^

Inline functions termination without return value

These are bugs. The appropriate action to take for the unimplemented cases is probably to throw an exception. However, i do not see exceptions implemented anywehere else in WCSim. So, for now, include a message for the unimplemented cases and return a nullptr.

Unused variable

In https://github.com/WCSim/WCSim/blob/0403225ffb3b22ef25113bd0ae66ab21db952699/src/WCSimRunAction.cc#L95, the creation of the branch on the RHS is necessary, but the pointer returned by function is never used directly. Just remove the variable declaration, keeping the rhs function call.

Conclusion

After these changes, there are no more compiler warnings from WCSimRunAction.

Warnings in WCSimEventAction

I looked at most of these in the discarded Issue spradlin/WCSim#2.

Warning text

Compiling WCSimEventAction.cc ...
In file included from /data/neutrino06/pspradlin/hk-hyperk/Geant4/install/include/Geant4/G4TrajectoryContainer.hh:46:0,
                 from include/WCSimEventAction.hh:11,
                 from src/WCSimEventAction.cc:1:
/data/neutrino06/pspradlin/hk-hyperk/Geant4/install/include/Geant4/G4VTrajectory.hh:91:17: warning: ‘virtual void G4VTrajectory::DrawTrajectory() const’ was hidden [-Woverloaded-virtual]
    virtual void DrawTrajectory() const;
                 ^
In file included from src/WCSimEventAction.cc:2:0:
include/WCSimTrajectory.hh:78:17: warning:   by ‘virtual void WCSimTrajectory::DrawTrajectory(G4int) const’ [-Woverloaded-virtual]
    virtual void DrawTrajectory(G4int i_mode=0) const;
                 ^
src/WCSimEventAction.cc: In member function ‘virtual void WCSimEventAction::BeginOfEventAction(const G4Event*)’:
src/WCSimEventAction.cc:175:20: warning: unused variable ‘DMman’ [-Wunused-variable]
     G4DigiManager* DMman = G4DigiManager::GetDMpointer();
                    ^
src/WCSimEventAction.cc: In member function ‘virtual void WCSimEventAction::EndOfEventAction(const G4Event*)’:
src/WCSimEventAction.cc:209:21: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
   for( Int_t u=0; u<nvtxs; u++ ){
                     ^
src/WCSimEventAction.cc:413:22: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
    for( Int_t u=0; u<nvtxs; u++ ){
                      ^
src/WCSimEventAction.cc:431:22: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
    for( Int_t u=0; u<nvtxs; u++ ){
                      ^
src/WCSimEventAction.cc:525:21: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
   for( Int_t u=0; u<nvtxs; u++ ){
                     ^
src/WCSimEventAction.cc: In member function ‘void WCSimEventAction::FillRootEvent(G4int, const ntupleStruct&, G4TrajectoryContainer*, WCSimWCDigitsCollection*, WCSimWCTriggeredDigitsCollection*, G4String)’:
src/WCSimEventAction.cc:808:62: warning: declaration of ‘jhfNtuple’ shadows a global declaration [-Wshadow]
                                      G4String detectorElement)
                                                              ^
In file included from include/WCSimRootEvent.hh:17:0,
                 from include/WCSimRunAction.hh:11,
                 from src/WCSimEventAction.cc:3:
include/jhfNtuple.h:55:28: warning: shadowed declaration is here [-Wshadow]
 extern struct ntupleStruct jhfNtuple;
                            ^
src/WCSimEventAction.cc:1083:12: warning: unused variable ‘hit_time_smear’ [-Wunused-variable]
     double hit_time_smear, hit_time_true;
            ^
src/WCSimEventAction.cc:1215:10: warning: unused variable ‘tree’ [-Wunused-variable]
   TTree* tree = GetRunAction()->GetTree();
          ^

The first couple of warnings are knock-ons from those in WCSimTrajectory (see previous comment).

Signed-unsigned comparisons

Each of these occurs in the context of a for loop in which the incremented loop variable is signed while the termination counter is unsigned.

First occurance

The code in context is at https://github.com/WCSim/WCSim/blob/0403225ffb3b22ef25113bd0ae66ab21db952699/src/WCSimEventAction.cc#L205-L213.

The termination counter is the return value of WCSimPrimaryGeneratorAction::GetNvtxs(), which returns a G4int. Change the type of both the termination counter and the loop variable to G4int.

Remaining occurances

For each of these, the termination counter nvtxs is the same as that for the first instance. So, these loop variables should also be changed to type G4int.

Knock-on effect

By changing the type of nvtxs to G4int, a new signed-unsigned comparision warning emerged for https://github.com/WCSim/WCSim/blob/0403225ffb3b22ef25113bd0ae66ab21db952699/src/WCSimEventAction.cc#L410. Also change its loop variable to type G4int.

Unused variables

WCSimEventAction::BeginOfEventAction() never uses the pointer to the G4DigiManager that it obtains, so that variable definition can be removed.

WCSimEventAction::FillRootEvent() only uses hit_time_smear in some optionally compiled code. That optionally compiled code can be rearranged to obviate hit_time_smear, so i did that.

The tree pointer obtained at https://github.com/WCSim/WCSim/blob/0403225ffb3b22ef25113bd0ae66ab21db952699/src/WCSimEventAction.cc#L1215 is vestigial. The code that used it is commented out below that. I commented out the line and moved it adjacent to the commented-out code that used it. Another candidate for cleanup.

Shadowing jhfNtuple

This is interesting. There is a global instance, which is used in WCSimEventAction::EndOfEventAction(). However, WCSimEventAction::FillRootEvent() expects an argument of the same name and type. The intent seems pretty clear. The argument to WCSimEventAction::FillRootEvent() is declared as a const &, which suggests an effort to prevent modification of the global instance by the method. So, rename the argument and update the body of WCSimEventAction::FillRootEvent() accordingly.

Testing with the optional compilation flags

There several blocks that are optionally compiled depending the values of various #define macros. Run a couple of tests with some of them activated

`_SAVE_RAW_HITS_VERBOSE`

Uncommenting #define _SAVE_RAW_HITS_VERBOSE uncovers a new warning:

src/WCSimEventAction.cc: In member function ‘void WCSimEventAction::FillRootEvent(G4int, const ntupleStruct&, G4TrajectoryContainer*, WCSimWCDigitsCollection*, WCSimWCTriggeredDigitsCollection*, G4String)’:
src/WCSimEventAction.cc:1098:39: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
  for(G4int id = 0; id < truetime.size(); id++) {
                                       ^

truetime is a std::vector<double>. Its size() method returns a std::size_t. Changing the loop variable to an unsigned fixes the warning.

`SAVE_DIGITS_VERBOSE`

Compilation fails if #define SAVE_DIGITS_VERBOSE is ucommented:

src/WCSimEventAction.cc: In member function ‘void WCSimEventAction::FillRootEvent(G4int, const ntupleStruct&, G4TrajectoryContainer*, WCSimWCDigitsCollection*, WCSimWCTriggeredDigitsCollection*, G4String)’:
src/WCSimEventAction.cc:1143:32: error: ‘digi_tubeid’ was not declared in this scope
   if(tubeID < NPMTS_VERBOSE || digi_tubeid == VERBOSE_PMT) {
                                ^

The variable digi_tubeid is declared at https://github.com/WCSim/WCSim/blob/0403225ffb3b22ef25113bd0ae66ab21db952699/src/WCSimEventAction.cc#L1087, inside the scope of a for loop that ends at https://github.com/WCSim/WCSim/blob/0403225ffb3b22ef25113bd0ae66ab21db952699/src/WCSimEventAction.cc#L1117.

Comparison with line https://github.com/WCSim/WCSim/blob/0403225ffb3b22ef25113bd0ae66ab21db952699/src/WCSimEventAction.cc#L1099 suggests that the variable tubeID should be used in both comparisons in the conditional, so replace the errant digi_tubeid with tubeID. That allows compilation without any new warnings.

`TIME_DAQ_STEPS`

Ucommenting #define TIME_DAQ_STEPS uncovers a units-shadowing warning:

src/WCSimEventAction.cc: In member function ‘virtual void WCSimEventAction::EndOfEventAction(const G4Event*)’:
src/WCSimEventAction.cc:259:15: warning: declaration of ‘ms’ shadows a global declaration [-Wshadow]
   TStopwatch* ms = new TStopwatch();
               ^
In file included from /data/neutrino06/pspradlin/hk-hyperk/Geant4/install/include/Geant4/CLHEP/Units/PhysicalConstants.h:42:0,
                 from /data/neutrino06/pspradlin/hk-hyperk/Geant4/install/include/Geant4/CLHEP/Vector/RotationX.icc:11,
                 from /data/neutrino06/pspradlin/hk-hyperk/Geant4/install/include/Geant4/CLHEP/Vector/RotationX.h:282,
                 from /data/neutrino06/pspradlin/hk-hyperk/Geant4/install/include/Geant4/CLHEP/Vector/Rotation.h:29,
                 from /data/neutrino06/pspradlin/hk-hyperk/Geant4/install/include/Geant4/CLHEP/Geometry/Transform3D.icc:6,
                 from /data/neutrino06/pspradlin/hk-hyperk/Geant4/install/include/Geant4/CLHEP/Geometry/Transform3D.h:818,
                 from /data/neutrino06/pspradlin/hk-hyperk/Geant4/install/include/Geant4/G4Transform3D.hh:33,
                 from include/WCSimDetectorConstruction.hh:9,
                 from include/WCSimEventAction.hh:10,
                 from src/WCSimEventAction.cc:1:
/data/neutrino06/pspradlin/hk-hyperk/Geant4/install/include/Geant4/CLHEP/Units/SystemOfUnits.h:142:23: warning: shadowed declaration is here [-Wshadow]
   static const double ms = millisecond;
                       ^

Change the name of the stopwatch pointer to sw.

Conclusion

After these changes, there are no more compiler warnings directly from WCSimEventAction.

Warnings in WCSimWCTrigger

Warning text

Compiling WCSimWCTrigger.cc ...
src/WCSimWCTrigger.cc: In constructor ‘WCSimWCTriggerBase::WCSimWCTriggerBase(G4String, WCSimDetectorConstruction*, WCSimWCDAQMessenger*, G4String)’:
src/WCSimWCTrigger.cc:41:36: warning: declaration of ‘detectorElement’ shadows a member of 'this' [-Wshadow]
            G4String detectorElement)
                                    ^
In file included from src/WCSimWCTrigger.cc:1:0:
include/WCSimWCTrigger.hh:173:12: warning: ‘WCSimWCTriggerBase::triggerClassName’ will be initialized after [-Wreorder]
   G4String triggerClassName; ///< Save the name of the trigger class
            ^
include/WCSimWCTrigger.hh:147:12: warning:   ‘G4String WCSimWCTriggerBase::detectorElement’ [-Wreorder]
   G4String detectorElement;
            ^
src/WCSimWCTrigger.cc:38:1: warning:   when initialized here [-Wreorder]
 WCSimWCTriggerBase::WCSimWCTriggerBase(G4String name,
 ^
src/WCSimWCTrigger.cc: At global scope:
src/WCSimWCTrigger.cc:478:6: warning: unused parameter ‘test’ [-Wunused-parameter]
 void WCSimWCTriggerBase::AlgNoTrigger(WCSimWCDigitsCollection* WCDCPMT, bool remove_hits, bool test) {
      ^
src/WCSimWCTrigger.cc: In constructor ‘WCSimWCTriggerNDigits::WCSimWCTriggerNDigits(G4String, WCSimDetectorConstruction*, WCSimWCDAQMessenger*, G4String)’:
src/WCSimWCTrigger.cc:583:35: warning: declaration of ‘detectorElement’ shadows a member of 'this' [-Wshadow]
           G4String detectorElement)
                                   ^
src/WCSimWCTrigger.cc:583:35: warning: declaration of ‘myDetector’ shadows a member of 'this' [-Wshadow]
src/WCSimWCTrigger.cc: In constructor ‘WCSimWCTriggerNoTrigger::WCSimWCTriggerNoTrigger(G4String, WCSimDetectorConstruction*, WCSimWCDAQMessenger*, G4String)’:
src/WCSimWCTrigger.cc:608:32: warning: declaration of ‘detectorElement’ shadows a member of 'this' [-Wshadow]
        G4String detectorElement)
                                ^
src/WCSimWCTrigger.cc:608:32: warning: declaration of ‘myDetector’ shadows a member of 'this' [-Wshadow]
src/WCSimWCTrigger.cc: In constructor ‘WCSimWCTriggerNDigits2::WCSimWCTriggerNDigits2(G4String, WCSimDetectorConstruction*, WCSimWCDAQMessenger*, G4String)’:
src/WCSimWCTrigger.cc:635:37: warning: declaration of ‘detectorElement’ shadows a member of 'this' [-Wshadow]
             G4String detectorElement)
                                     ^
src/WCSimWCTrigger.cc:635:37: warning: declaration of ‘myDetector’ shadows a member of 'this' [-Wshadow]

Data member initializer order

Should be the same as their order of declaration. Fixed by minor rearrangement of the initializer list.

Shadowing `detectorElement`

Adopted the conventional solution of updating the names of all private and protected data members to begin with f.

Unused parameter

The function WCSimWCTriggerBase::AlgNoTrigger() indeed never use its argurment test. It looks like the argument is there to be consistent with the signatures of WCSimWCTriggerBase::AlgNDigits(), or that it is an unimplemented feature.

There should be follow-up on the appropriate behavior for WCSimWCTriggerBase::AlgNoTrigger() when it is passed a test value of true. For now, i will code a message stating that the test feature is not implemented.

Testing with the optional compilation flags

`WCSIMWCTRIGGER_VERBOSE`

Uncommenting #define WCSIMWCTRIGGER_VERBOSE does not uncover new compiler warnings.

`HYPER_VERBOSITY`

Ucommenting #define HYPER_VERBOSITY does not uncover new compiler warnings.

Conclusion

After these changes, there are no more compiler warnings directly from WCSimWCTrigger.

Warnings in WCSimConstructCylinder.cc

src/WCSimConstructCylinder.cc is an implementation file for the WCSimDetectorConstruction class. This component has more warnings than any other.

Warning text

Compiling WCSimConstructCylinder.cc ...
src/WCSimConstructCylinder.cc: In member function ‘G4LogicalVolume* WCSimDetectorConstruction::ConstructCylinder()’:
src/WCSimConstructCylinder.cc:191:24: warning: unused variable ‘physiCaveTyvek’ [-Wunused-variable]
     G4VPhysicalVolume *physiCaveTyvek =
                        ^
src/WCSimConstructCylinder.cc:200:27: warning: unused variable ‘TyvekCaveBarrelSurface’ [-Wunused-variable]
     G4LogicalSkinSurface *TyvekCaveBarrelSurface = new G4LogicalSkinSurface("TyvekCaveBarrelSurface", logicCaveTyvek, OpWaterTySurface);
                           ^
src/WCSimConstructCylinder.cc:224:27: warning: unused variable ‘TyvekCaveTopSurface’ [-Wunused-variable]
     G4LogicalSkinSurface *TyvekCaveTopSurface = new G4LogicalSkinSurface("TyvekCaveTopSurface", logicCaveCapsTyvek, OpWaterTySurface);
                           ^
src/WCSimConstructCylinder.cc:233:24: warning: unused variable ‘physiTopCaveTyvek’ [-Wunused-variable]
     G4VPhysicalVolume *physiTopCaveTyvek =
                        ^
src/WCSimConstructCylinder.cc:245:24: warning: unused variable ‘physiBottomCaveTyvek’ [-Wunused-variable]
     G4VPhysicalVolume *physiBottomCaveTyvek =
                        ^
src/WCSimConstructCylinder.cc:492:24: warning: unused variable ‘physiWCExtraTower’ [-Wunused-variable]
     G4VPhysicalVolume* physiWCExtraTower = 
                        ^
src/WCSimConstructCylinder.cc:564:30: warning: unused variable ‘WaterBSTowerCellSurface’ [-Wunused-variable]
     G4LogicalBorderSurface * WaterBSTowerCellSurface 
                              ^
src/WCSimConstructCylinder.cc:634:23: warning: unused variable ‘physiWCTopVeto’ [-Wunused-variable]
    G4VPhysicalVolume* physiWCTopVeto =
                       ^
src/WCSimConstructCylinder.cc:663:23: warning: unused variable ‘physiWCTVTyvekBot’ [-Wunused-variable]
    G4VPhysicalVolume* physiWCTVTyvekBot =
                       ^
src/WCSimConstructCylinder.cc:672:33: warning: unused variable ‘WaterTyTVSurfaceBot’ [-Wunused-variable]
           G4LogicalSkinSurface *WaterTyTVSurfaceBot = new G4LogicalSkinSurface("WaterTyTVSurfaceBot", logicWCTVTyvek, OpWaterTySurface);
                                 ^
src/WCSimConstructCylinder.cc:674:23: warning: unused variable ‘physiWCTVTyvekTop’ [-Wunused-variable]
    G4VPhysicalVolume* physiWCTVTyvekTop =
                       ^
src/WCSimConstructCylinder.cc:701:23: warning: unused variable ‘physiWCTVTyvekSide’ [-Wunused-variable]
    G4VPhysicalVolume* physiWCTVTyvekSide =
                       ^
src/WCSimConstructCylinder.cc:709:33: warning: unused variable ‘WaterTyTVSurfaceSurface’ [-Wunused-variable]
           G4LogicalSkinSurface *WaterTyTVSurfaceSurface = new G4LogicalSkinSurface("WaterTyTVSurfaceSide", logicWCTVTyvekSide, OpWaterTySurface);
                                 ^
src/WCSimConstructCylinder.cc:766:26: warning: unused variable ‘physiCapPMT’ [-Wunused-variable]
       G4VPhysicalVolume* physiCapPMT =
                          ^
src/WCSimConstructCylinder.cc:806:26: warning: unused variable ‘physiWCBarrelPMT’ [-Wunused-variable]
       G4VPhysicalVolume* physiWCBarrelPMT =
                          ^
src/WCSimConstructCylinder.cc:829:23: warning: declaration of ‘WCPMTRotation’ shadows a previous local [-Wshadow]
     G4RotationMatrix* WCPMTRotation = new G4RotationMatrix;
                       ^
src/WCSimConstructCylinder.cc:792:21: warning: shadowed declaration is here [-Wshadow]
   G4RotationMatrix* WCPMTRotation = new G4RotationMatrix;
                     ^
src/WCSimConstructCylinder.cc:835:14: warning: declaration of ‘horizontalSpacing’ shadows a previous local [-Wshadow]
     G4double horizontalSpacing   = towerWidth/(WCBarrelNumPMTHorizontal-WCBarrelRingNPhi*WCPMTperCellHorizontal);
              ^
src/WCSimConstructCylinder.cc:796:12: warning: shadowed declaration is here [-Wshadow]
   G4double horizontalSpacing   = barrelCellWidth/WCPMTperCellHorizontal;
            ^
src/WCSimConstructCylinder.cc:836:14: warning: declaration of ‘verticalSpacing’ shadows a previous local [-Wshadow]
     G4double verticalSpacing     = barrelCellHeight/WCPMTperCellVertical;
              ^
src/WCSimConstructCylinder.cc:797:12: warning: shadowed declaration is here [-Wshadow]
   G4double verticalSpacing     = barrelCellHeight/WCPMTperCellVertical;
            ^
src/WCSimConstructCylinder.cc:847:21: warning: unused variable ‘physiWCBarrelPMT’ [-Wunused-variable]
  G4VPhysicalVolume* physiWCBarrelPMT =
                     ^
src/WCSimConstructCylinder.cc:1042:28: warning: unused variable ‘physiWCBarrelWLSPlate’ [-Wunused-variable]
         G4VPhysicalVolume* physiWCBarrelWLSPlate =
                            ^
src/WCSimConstructCylinder.cc:1107:25: warning: declaration of ‘WCPMTRotation’ shadows a previous local [-Wshadow]
       G4RotationMatrix* WCPMTRotation = new G4RotationMatrix;
                         ^
src/WCSimConstructCylinder.cc:792:21: warning: shadowed declaration is here [-Wshadow]
   G4RotationMatrix* WCPMTRotation = new G4RotationMatrix;
                     ^
src/WCSimConstructCylinder.cc:1115:16: warning: declaration of ‘horizontalODSpacing’ shadows a previous local [-Wshadow]
       G4double horizontalODSpacing   = towerWidthOD/(double)WCPMTODperCellHorizontalExtra;
                ^
src/WCSimConstructCylinder.cc:1028:14: warning: shadowed declaration is here [-Wshadow]
     G4double horizontalODSpacing = barrelODCellWidth/WCPMTODperCellHorizontal;
              ^
src/WCSimConstructCylinder.cc:1116:16: warning: declaration of ‘verticalODSpacing’ shadows a previous local [-Wshadow]
       G4double verticalODSpacing   = barrelODCellHeight/WCPMTODperCellVertical;
                ^
src/WCSimConstructCylinder.cc:1029:14: warning: shadowed declaration is here [-Wshadow]
     G4double verticalODSpacing   = barrelODCellHeight/WCPMTODperCellVertical;
              ^
src/WCSimConstructCylinder.cc:1126:23: warning: unused variable ‘physiWCExtraBarrelWLSPlate’ [-Wunused-variable]
    G4VPhysicalVolume* physiWCExtraBarrelWLSPlate =
                       ^
src/WCSimConstructCylinder.cc:1087:29: warning: unused variable ‘WaterExtraTySurfaceSide’ [-Wunused-variable]
       G4LogicalSkinSurface *WaterExtraTySurfaceSide =
                             ^
src/WCSimConstructCylinder.cc:1096:26: warning: unused variable ‘physiWCTowerODTyvek’ [-Wunused-variable]
       G4VPhysicalVolume* physiWCTowerODTyvek =
                          ^
src/WCSimConstructCylinder.cc:1176:23: warning: unused variable ‘physiTopCapWLSPlate’ [-Wunused-variable]
    G4VPhysicalVolume* physiTopCapWLSPlate =
                       ^
src/WCSimConstructCylinder.cc:1186:23: warning: unused variable ‘physiBottomCapWLSPlate’ [-Wunused-variable]
    G4VPhysicalVolume* physiBottomCapWLSPlate =
                       ^
src/WCSimConstructCylinder.cc:913:24: warning: unused variable ‘physiWCODTopCapsTyvek’ [-Wunused-variable]
     G4VPhysicalVolume* physiWCODTopCapsTyvek =
                        ^
src/WCSimConstructCylinder.cc:922:27: warning: unused variable ‘WaterTySurfaceTop’ [-Wunused-variable]
     G4LogicalSkinSurface *WaterTySurfaceTop =
                           ^
src/WCSimConstructCylinder.cc:927:24: warning: unused variable ‘physiWCODBottomCapsTyvek’ [-Wunused-variable]
     G4VPhysicalVolume* physiWCODBottomCapsTyvek =
                        ^
src/WCSimConstructCylinder.cc:972:24: warning: unused variable ‘physiWCBarrelCellODTyvek’ [-Wunused-variable]
     G4VPhysicalVolume* physiWCBarrelCellODTyvek =
                        ^
src/WCSimConstructCylinder.cc:980:27: warning: unused variable ‘WaterTySurfaceSide’ [-Wunused-variable]
     G4LogicalSkinSurface *WaterTySurfaceSide =
                           ^
src/WCSimConstructCylinder.cc:164:24: warning: unused variable ‘physiWCBarrel’ [-Wunused-variable]
     G4VPhysicalVolume* physiWCBarrel = 
                        ^
src/WCSimConstructCylinder.cc:292:22: warning: unused variable ‘physiWCBarrelAnnulus’ [-Wunused-variable]
   G4VPhysicalVolume* physiWCBarrelAnnulus = 
                      ^
src/WCSimConstructCylinder.cc:323:22: warning: unused variable ‘physiWCBarrelRing’ [-Wunused-variable]
   G4VPhysicalVolume* physiWCBarrelRing = 
                      ^
src/WCSimConstructCylinder.cc:423:28: warning: unused variable ‘WaterBSBarrelCellSurface’ [-Wunused-variable]
   G4LogicalBorderSurface * WaterBSBarrelCellSurface 
                            ^
src/WCSimConstructCylinder.cc:459:20: warning: unused variable ‘logicWCExtraBorderCell’ [-Wunused-variable]
   G4LogicalVolume* logicWCExtraBorderCell;
                    ^
src/WCSimConstructCylinder.cc:1217:22: warning: unused variable ‘physiTopCapAssembly’ [-Wunused-variable]
   G4VPhysicalVolume* physiTopCapAssembly =
                      ^
src/WCSimConstructCylinder.cc:1225:22: warning: unused variable ‘physiBottomCapAssembly’ [-Wunused-variable]
   G4VPhysicalVolume* physiBottomCapAssembly =
                      ^
src/WCSimConstructCylinder.cc: In member function ‘G4LogicalVolume* WCSimDetectorConstruction::ConstructCaps(G4int)’:
src/WCSimConstructCylinder.cc:1328:26: warning: declaration of ‘tmpVisAtt’ shadows a previous local [-Wshadow]
         G4VisAttributes* tmpVisAtt = new G4VisAttributes(G4Colour(1.,0.5,0.5));
                          ^
src/WCSimConstructCylinder.cc:1255:20: warning: shadowed declaration is here [-Wshadow]
   G4VisAttributes* tmpVisAtt = new G4VisAttributes(G4VisAttributes::Invisible);
                    ^
src/WCSimConstructCylinder.cc:1333:26: warning: declaration of ‘tmpVisAtt’ shadows a previous local [-Wshadow]
         G4VisAttributes* tmpVisAtt = new G4VisAttributes(G4Colour(1.,0.5,0.5));
                          ^
src/WCSimConstructCylinder.cc:1255:20: warning: shadowed declaration is here [-Wshadow]
   G4VisAttributes* tmpVisAtt = new G4VisAttributes(G4VisAttributes::Invisible);
                    ^
src/WCSimConstructCylinder.cc:1344:26: warning: declaration of ‘tmpVisAtt’ shadows a previous local [-Wshadow]
         G4VisAttributes* tmpVisAtt = new G4VisAttributes(G4Colour(1.,0.5,0.5));
                          ^
src/WCSimConstructCylinder.cc:1255:20: warning: shadowed declaration is here [-Wshadow]
   G4VisAttributes* tmpVisAtt = new G4VisAttributes(G4VisAttributes::Invisible);
                    ^
src/WCSimConstructCylinder.cc:1424:30: warning: unused variable ‘WaterBSExtraBorderCellSurface’ [-Wunused-variable]
     G4LogicalBorderSurface * WaterBSExtraBorderCellSurface 
                              ^
src/WCSimConstructCylinder.cc:1609:21: warning: declaration of ‘WCCapBlackSheetVisAtt’ shadows a previous local [-Wshadow]
    G4VisAttributes* WCCapBlackSheetVisAtt 
                     ^
src/WCSimConstructCylinder.cc:1603:21: warning: shadowed declaration is here [-Wshadow]
    G4VisAttributes* WCCapBlackSheetVisAtt 
                     ^
src/WCSimConstructCylinder.cc:1620:21: warning: declaration of ‘WCCapBlackSheetVisAtt’ shadows a previous local [-Wshadow]
    G4VisAttributes* WCCapBlackSheetVisAtt 
                     ^
src/WCSimConstructCylinder.cc:1603:21: warning: shadowed declaration is here [-Wshadow]
    G4VisAttributes* WCCapBlackSheetVisAtt 
                     ^
src/WCSimConstructCylinder.cc:1664:21: warning: unused variable ‘physiCapPMT’ [-Wunused-variable]
  G4VPhysicalVolume* physiCapPMT =
                     ^
src/WCSimConstructCylinder.cc:1699:25: warning: unused variable ‘physiWCBarrelBorderPMT’ [-Wunused-variable]
      G4VPhysicalVolume* physiWCBarrelBorderPMT =
                         ^
src/WCSimConstructCylinder.cc:1722:23: warning: declaration of ‘WCPMTRotation’ shadows a previous local [-Wshadow]
     G4RotationMatrix* WCPMTRotation = new G4RotationMatrix;
                       ^
src/WCSimConstructCylinder.cc:1686:21: warning: shadowed declaration is here [-Wshadow]
   G4RotationMatrix* WCPMTRotation = new G4RotationMatrix;
                     ^
src/WCSimConstructCylinder.cc:1728:14: warning: declaration of ‘horizontalSpacing’ shadows a previous local [-Wshadow]
     G4double horizontalSpacing   = towerWidth/(WCBarrelNumPMTHorizontal-WCBarrelRingNPhi*WCPMTperCellHorizontal);
              ^
src/WCSimConstructCylinder.cc:1690:12: warning: shadowed declaration is here [-Wshadow]
   G4double horizontalSpacing   = barrelCellWidth/WCPMTperCellHorizontal;
            ^
src/WCSimConstructCylinder.cc:1729:14: warning: declaration of ‘verticalSpacing’ shadows a previous local [-Wshadow]
     G4double verticalSpacing     = (barrelCellHeight-WCBorderPMTOffset)/WCPMTperCellVertical;
              ^
src/WCSimConstructCylinder.cc:1691:12: warning: shadowed declaration is here [-Wshadow]
   G4double verticalSpacing     = (barrelCellHeight-WCBorderPMTOffset)/WCPMTperCellVertical;
            ^
src/WCSimConstructCylinder.cc:1739:21: warning: unused variable ‘physiWCBarrelBorderPMT’ [-Wunused-variable]
  G4VPhysicalVolume* physiWCBarrelBorderPMT =
                     ^
src/WCSimConstructCylinder.cc:1805:28: warning: unused variable ‘physiWCBarrelWLSPlate’ [-Wunused-variable]
         G4VPhysicalVolume* physiWCBarrelWLSPlate =
                            ^
src/WCSimConstructCylinder.cc:1831:16: warning: declaration of ‘barrelODCellWidth’ shadows a previous local [-Wshadow]
       G4double barrelODCellWidth   = 2.*WCODRadius*tan(dPhi/2.);
                ^
src/WCSimConstructCylinder.cc:1793:14: warning: shadowed declaration is here [-Wshadow]
     G4double barrelODCellWidth   = 2.*WCODRadius*tan(dPhi/2.);
              ^
src/WCSimConstructCylinder.cc:1832:16: warning: declaration of ‘barrelODCellHeight’ shadows a previous local [-Wshadow]
       G4double barrelODCellHeight  = barrelCellHeight * (barrelODCellWidth/barrelCellWidth);
                ^
src/WCSimConstructCylinder.cc:1794:14: warning: shadowed declaration is here [-Wshadow]
     G4double barrelODCellHeight  = barrelCellHeight * (barrelODCellWidth/barrelCellWidth);
              ^
src/WCSimConstructCylinder.cc:1834:25: warning: declaration of ‘WCPMTRotation’ shadows a previous local [-Wshadow]
       G4RotationMatrix* WCPMTRotation = new G4RotationMatrix;
                         ^
src/WCSimConstructCylinder.cc:1686:21: warning: shadowed declaration is here [-Wshadow]
   G4RotationMatrix* WCPMTRotation = new G4RotationMatrix;
                     ^
src/WCSimConstructCylinder.cc:1842:16: warning: declaration of ‘horizontalODSpacing’ shadows a previous local [-Wshadow]
       G4double horizontalODSpacing   = towerWidthOD/(double)WCPMTODperCellHorizontalExtra;
                ^
src/WCSimConstructCylinder.cc:1795:14: warning: shadowed declaration is here [-Wshadow]
     G4double horizontalODSpacing = barrelODCellWidth/WCPMTODperCellHorizontal;
              ^
src/WCSimConstructCylinder.cc:1843:16: warning: declaration of ‘verticalODSpacing’ shadows a previous local [-Wshadow]
       G4double verticalODSpacing   = barrelODCellHeight/WCPMTODperCellVertical;
                ^
src/WCSimConstructCylinder.cc:1796:14: warning: shadowed declaration is here [-Wshadow]
     G4double verticalODSpacing   = barrelODCellHeight / WCPMTODperCellVertical;
              ^
src/WCSimConstructCylinder.cc:1853:30: warning: unused variable ‘physiWCBarrelPMT’ [-Wunused-variable]
           G4VPhysicalVolume* physiWCBarrelPMT =
                              ^
src/WCSimConstructCylinder.cc:1822:26: warning: unused variable ‘physiWCExtraBorderODTyvek’ [-Wunused-variable]
       G4VPhysicalVolume* physiWCExtraBorderODTyvek =
                          ^
src/WCSimConstructCylinder.cc:1775:24: warning: unused variable ‘physiWCBarrelBorderCellODTyvek’ [-Wunused-variable]
     G4VPhysicalVolume* physiWCBarrelBorderCellODTyvek =
                        ^
src/WCSimConstructCylinder.cc:1283:22: warning: unused variable ‘physiWCBarrelBorderRing’ [-Wunused-variable]
   G4VPhysicalVolume* physiWCBarrelBorderRing =
                      ^
src/WCSimConstructCylinder.cc:1365:28: warning: unused variable ‘WaterBSBarrelBorderCellSurface’ [-Wunused-variable]
   G4LogicalBorderSurface * WaterBSBarrelBorderCellSurface
                            ^
src/WCSimConstructCylinder.cc:1373:20: warning: unused variable ‘logicWCExtraTowerCell’ [-Wunused-variable]
   G4LogicalVolume* logicWCExtraTowerCell;
                    ^
src/WCSimConstructCylinder.cc:1598:29: warning: unused variable ‘WaterBSBottomCapSurface’ [-Wunused-variable]
    G4LogicalBorderSurface * WaterBSBottomCapSurface 
                             ^
src/WCSimConstructCylinder.cc:1603:21: warning: unused variable ‘WCCapBlackSheetVisAtt’ [-Wunused-variable]
    G4VisAttributes* WCCapBlackSheetVisAtt 
                     ^

Unused variables

There are many, many unused variables. Most (perhaps all) of these are unused pointers to dynamically allocated volume elements. The declarations of most of these can be knocked out leaving the allocation call, as i did for src/WCSimConstructPMT.cc. This doesn't seem like a proper fix. Should revisit this.

Shadowing warnings

The complexity of the scopes in huge methods and the reuse of variable names looks thorny. It is relatively straightforward to eliminates the conflicts in a way that preserves the current behavior. It is much less straightforward to determine whether that is the intended behavior.

First group

The first group of shadowed variables appear in the same conditional block:

WCPMTRotation at line 829
horizontalSpacing at line 835
verticalSpacing at line 836

Each of these variables shares a name with a variable of the same type one scope up, outside of the conditional. In this case, it is clear that everything within the conditional block should use the locally scoped versions, which it does. However, to make it clearer and to eliminate the warnings, i renamed these variables to indicate that they are parameters associated with the ExtraTower of PMTs that is conditionally constructed.

Second group

The second group looks almost identical:

WCPMTRotation at line 1107
'horizontalODSpacing' at line 1115
verticalODSpacing at line 1116

Their function appear to be compleletly analagous to the first group, save these are for an ExtraTower of PMTs in the Outer Detector rather than in the main tank. As with the first group, the intent is clear, so i changed the names of the variables in the inner scope to indicate their association with the ExtraTower.

The tmpVisAtt's

The next tranch of shadowing warnings refer to a number of visualization attributes that are created in various internal scopes. I note that the conflict warnings are avoided in https://github.com/WCSim/WCSim/blob/0403225ffb3b22ef25113bd0ae66ab21db952699/src/WCSimConstructCylinder.cc#L1508-L1528 by using variable name with a numeric suffix, tmpVisAtt2. So, i renamed all of the internal scope variables with conflicts to tmpVisAtt1.

Fourth group

It looks like another group of duplicated variables for an ExtraTower. Perform the same renaming.

Fifth group

These have different names, but they, again, look like variables for a special treatment of an extra tower in the OD barrel:

barrelODCellWidth at line 1831
barrelODCellHeight at line 1832
WCPMTRotation at line 1834
horizontalODSpacing at line 1842
verticalODSpacing at line 1843

Again, change the names of the inner scope to reflect association with the extra tower.

Conclusion

After these changes, there are no more compiler warnings from WCSimConstructCylinder.cc. However, some follow-up and cleaning is indicated.

Warnings in WCSimPMTObject

Warning text

Compiling WCSimPMTObject.cc ...
src/WCSimPMTObject.cc: In member function ‘void WCSimBasicPMTObject::DefineQEHist(std::map<double, double>)’:
src/WCSimPMTObject.cc:2045:73: warning: declaration of ‘mapQE’ shadows a member of 'this' [-Wshadow]
 void WCSimBasicPMTObject::DefineQEHist(std::map<G4double,G4double> mapQE){

Shadowing

Adopted the conventional solution of updating the names of all private and protected data members to begin with f.

Conclusion

After these changes, there are no more compiler warnings from WCSimPMTObject.

It looks like the implementation of WCSimBasicPMTObject may be incomplete. It maintains in parallel two representations of the quantum efficiency curves, but these may not be maintained consistently.

spradlin / WCSim