Variable sized arrays/vectors include size in name - breaks output because Find() fails

[leadmocha]

Both VarriableArrayVar and VectorVar leave the square brackets in the name where as FixedArrayVar removes it. For example

OBJ: THaVar sbs.hcal.a.m0[250] Raw ADC samples

for a VectorVar/VarriableArrayVar vs

OBJ: THaVar sbs.hcal.a.m0 Raw ADC samples

for a FixedArrayVar. The consequence of this is that THaVarList::Find() will not find these variables in the list leading to errors such as

THaOutput::Init: WARNING: Global variable sbs.hcal.a.m0[250] does not exist.

and no output. In the above case the output is specified by a block statement block sbs.hcal.a.m* which is what adds the [250] when it expands the variable name. Similarly looking for it without the [250] yields the same error because ::Find() specifically ignores the brackets, and we still get an error like so

THaOutput::Init: WARNING: Global variable sbs.hcal.a.m0 does not match any variables.

Another consequence is that one is able to generate duplicate variables with that name because again Find() fails to find them so it does not recognize it as a duplicate entry.

I tentatively fixed it by copying code from THaVarList::Find() and tweaking it slightly to change the name on constructors of VarriableArrayVar and VectorVar:

const char* name = pvar->TNamed::GetName();                                      
const char* p = strchr( name, '[' );                                             
if( p ) {                                                                        
  size_t n = p-name;                                                             
  char* basename = new char[ n+1 ];                                              
  strncpy( basename, name, n );                                                  
  basename[n] = '\0';                                                            
  pvar->TNamed::SetName(basename);                                               
}                                                                                

Adding a SanatizeName() function Variable with similar functionality can work too. Would be nice to have this fixed on the main repo.

[hansenjo]

Hi Carlos, thanks for reporting this, especially with the example code to reproduce it. I'll look into it today. Ole

[hansenjo]

Hi again Carlos, actually, trying to define a variable with square brackets in the name as a VariableArrayVar or VectorVar should give an error. By definition, those are variable-size arrays, so what would the "[250]" mean? I think the bug is that those definitions are accepted without error, not that the brackets aren't removed somehow. Could you send me a snippet of the header file where the data members are defined, and of the DefineVariables function where the global variables are defined, or point me to the code of that module (presumably the SBS HCAL code)? Thanks, Ole

[leadmocha]

Hi Ole,

The brackets are not added by me, but rather by THaVarList::DefineVariables (line 499-500 in master).

In the SBS-offline repository in SBSHCal.cxx lines 264-293 is an example of how I define them. I have an updated version that just uses vectors since I've just discovered that the analyzer can handle vectors all on its own.

FixedArrayVar seems to know to remove the brackets before setting the name, but nothing in the constructor of VariableArrayVar and VectorVar presently tries to remove the brackets.

Thanks again for looking into this.

Best,
Juan Carlos

[leadmocha]

Hi again,

Just to answer your question: the 250 is the maximum size for that vector/array. I also agree that for variable sized objects it doesn't make sense to include it in the name. Maybe changing the way THaVarList::DefineVariables works may be the better way to fix it. i.e. by modifying line 483 to read

if ( item->size>1 && !item->count )

That ought to ensure the brackets are not added for variable sized arrays and still leave it in there for FixedVarArray (which seems to need it to determine the size of the array).

[leadmocha]

Oh, oops, that would only work for variable arrays but still fail for vectors....

[hansenjo]

Hi Carlos, thanks. I think I see the issue now. This function Int_t THaVarList::DefineVariables( const VarDef* list, const char* prefix, const char* caller ) is buggy indeed. It gets called from Int_t THaAnalysisObject::DefineVarsFromList( const VarDef* list, EMode mode, const char* var_prefix ) const Thing is, this is basically an obsolete interface. I actually thought I had already removed it. Instead, one should use the corresponding function with *RVarDef*: Int_t THaAnalysisObject::DefineVarsFromList( const RVarDef* list, EMode mode, const char* var_prefix ) const For SBS, I strongly recommend to switch to the *RVarDef* version. Because of legacy code, it may be a good idea to keep the VarDef interface in analyzer 1.6. So I'll work on a fix now. Ole

[leadmocha]

Oh I did not realize VarDef was on its way out. I would have used it except it doesn't seem to be able to handle 2d vectors. Each entry in the first vector is a different global variable/leaf. VarDef does let me specify that.

But perhaps I'll continue this discussion with you via email.

[hansenjo]

Copied to Redmine bug no 334

[hansenjo]

Hi Carlos,

I guess you saw that this bug has been fixed in the current version of the analyzer. Read the comments for commit d52843a for details.

The root cause of the incorrect variable definitions (with square brackets in their names) is line 279 in SBSHCal.cxx:

v.size = MAX_FADC_SAMPLES;

That should be

v.size = 0;

because v.count is also set, indicating that you want to define a variable-size array. The analyzer failed to handle this contradictory input properly.

The fixed code now detects this situation (fixed and a variable array requested at the same time?), ignores v.size, prints a warning, and defines a variable-size array with size v.count.

Variable-size arrays can be any size. There is no check on maximum array size because such a limit would be arbitrary, so the "size" parameter in VarDef is meaningless.

Hope that helps. Thanks again for discovering this flaw.

Ole