Page MenuHomeHEPForge
Differential D52 Diff 223 src/LauBlattWeisskopfFactor.cc

Changeset View

Standalone View

View Options

src/LauBlattWeisskopfFactor.cc

Show First 20 Lines • Show All 52 Lines • ▼ Show 20 LinesLauBlattWeisskopfFactor::LauBlattWeisskopfFactor( const LauResonanceInfo& resInfo, const Double_t resRadius, const BarrierType barrierType, const RestFrame restFrame, const BlattWeisskopfCategory category ) :
radius_(new LauParameter("NEED_A_GOOD_NAME",resRadius,0.0,10.0,kTRUE)), radius_(new LauParameter("NEED_A_GOOD_NAME",resRadius,0.0,10.0,kTRUE)),
barrierType_(barrierType), barrierType_(barrierType),
restFrame_(restFrame) restFrame_(restFrame)
{ {
TString categoryName = this->setRadiusName( resInfo, category ); TString categoryName = this->setRadiusName( resInfo, category );
std::cout << "INFO in LauBlattWeisskopfFactor constructor : creating radius parameter for category \"" << categoryName << "\", with initial value " << resRadius << std::endl; std::cout << "INFO in LauBlattWeisskopfFactor constructor : creating radius parameter for category \"" << categoryName << "\", with initial value " << resRadius << std::endl;
} }
LauBlattWeisskopfFactor::LauBlattWeisskopfFactor( const Int_t spin, const Double_t resRadius, const BarrierType barrierType, const RestFrame restFrame, const BlattWeisskopfCategory category ) :
spin_(spin),
radius_(new LauParameter("NEED_A_GOOD_NAME",resRadius,0.0,10.0,kTRUE)),
johndanAuthorUnsubmitted
Done
Inline Actions

Why was NEED_A_GOOD_NAME here?

johndan: Why was `NEED_A_GOOD_NAME` here?
tlathamUnsubmitted
Done
Inline Actions

I think it's just there as an explicit reminder that the name needs to be, and indeed is, updated in the function body.

tlatham: I think it's just there as an explicit reminder that the name needs to be, and indeed is…
barrierType_(barrierType),
restFrame_(restFrame)
{
TString categoryName = this->setRadiusName( category );
std::cout << "INFO in LauBlattWeisskopfFactor constructor : creating radius parameter for category \"" << categoryName << "\", with initial value " << resRadius << std::endl;
}
LauBlattWeisskopfFactor::~LauBlattWeisskopfFactor() LauBlattWeisskopfFactor::~LauBlattWeisskopfFactor()
{ {
} }
LauBlattWeisskopfFactor::LauBlattWeisskopfFactor( const LauBlattWeisskopfFactor& other, const UInt_t newSpin, const BarrierType newBarrierType ) : LauBlattWeisskopfFactor::LauBlattWeisskopfFactor( const LauBlattWeisskopfFactor& other, const UInt_t newSpin, const BarrierType newBarrierType ) :
spin_(newSpin), spin_(newSpin),
radius_(other.radius_->createClone()), radius_(other.radius_->createClone()),
barrierType_(newBarrierType), barrierType_(newBarrierType),
restFrame_(other.restFrame_) restFrame_(other.restFrame_)
{ {
} }
TString LauBlattWeisskopfFactor::setRadiusName( const LauResonanceInfo& resInfo, const BlattWeisskopfCategory category ) TString LauBlattWeisskopfFactor::setRadiusName( const LauResonanceInfo& resInfo, const BlattWeisskopfCategory category )
{ {
switch (category) {
case Indep :
johndanAuthorUnsubmitted
Done
Inline Actions

This seems to be the only case in which resInfo is actually needed, so for all the others I just use a default argument of "" for the category name and then it is assigned in the old way. Passing around "" seems imperfect, and perhaps you'll have a smarter way to do this.

What I would rather do is modify the original
TString setRadiusName( const LauResonanceInfo& resInfo, const BlattWeisskopfCategory category );
function in order to swap the parameter ordering and give resInfo a default argument that can be overridden for the case of Indep category. But I was afraid to redefine setRadiusName because it's not private to the class (even if it is protected and I don't see any instances in Laura++ anywhere, so changing it would probably be safe).

johndan: This seems to be the only case in which `resInfo` is actually needed, so for all the others I…
tlathamUnsubmitted
Done
Inline Actions

An alternative way would be to have 3 functions:

TString setRadiusName( const LauResonanceInfo& resInfo, const BlattWeisskopfCategory category );
TString setRadiusName( const BlattWeisskopfCategory category );
TString setRadiusName( const TString& categoryName );

The first is modified as you've done except that in the Indep case you just call the 3rd one with the result of resInfo.getSanitisedName().
The second one just contains the switch on the rest of the categories (although I'd prefer to remove the default case and replace it with the two cases for Default and Indep), in each case just calling the 3rd on with the category name.
The third then would just be:

TString LauBlattWeisskopfFactor::setRadiusName( const TString& categoryName )
{
    TString name = "BarrierRadius_";
    name.Append(categoryName);
    radius_->name(name);
    return categoryName;
}

This avoids having any default arguments but does rather proliferate the number of functions.

I agree that these functions should be private, and have no objection to them being moved into that access category.
So then the protected category can go, and we could even declare the class as final and get rid of the virtual destructor.

tlatham: An alternative way would be to have 3 functions: ``` TString setRadiusName( const…
johndanAuthorUnsubmitted
Done
Inline Actions

Thanks for the detailed proposal!

So for the second one, and case Indep:, what does the action look like? You don't have the resInfo to get a name from... Also, what should the name be for the "Default" case? "Unknown", like before, or something else?

johndan: Thanks for the detailed proposal! So for the second one, and `case Indep:`, what does the…
johndanAuthorUnsubmitted
Done
Inline Actions

Also, why would the second one call the third one, since both have the same form. Why not just set the categoryName variable for the Indep and Default cases?

johndan: Also, why would the second one call the third one, since both have the same form. Why not just…
tlathamUnsubmitted
Done
Inline Actions

Apologies, I accidentally omitted a few details. To avoid further ambiguities, here's exactly what I had in mind. Hopefully this clarifies what I meant to the extent that all your questions are answered?

TString LauBlattWeisskopfFactor::setRadiusName( const LauResonanceInfo& resInfo, const BlattWeisskopfCategory category )
{
        switch (category) {
                case Indep :
                        return this->setRadiusName( resInfo.getSanitisedName() );
                        break;
                default :
                        return this->setRadiusName( category );
                        break;
        }   
}

TString LauBlattWeisskopfFactor::setRadiusName( const BlattWeisskopfCategory category )
{
                switch (category) {
                        case Default :
                        case Indep :
                                // We shouldn't ever end up here
                                return this->setRadiusName("Unknown");
                        case Parent :
                                return this->setRadiusName("Parent");
                        case Light :
                                return this->setRadiusName("Light");
                        case Kstar :
                                return this->setRadiusName("Kstar");
                        case Charm :
                                return this->setRadiusName("Charm");
                        case StrangeCharm :
                                return this->setRadiusName("StrangeCharm");
                        case Charmonium :
                                return this->setRadiusName("Charmonium");
                        case Beauty :
                                return this->setRadiusName("Beauty");
                        case StrangeBeauty :
                                return this->setRadiusName("StrangeBeauty");
                        case CharmBeauty :
                                return this->setRadiusName("CharmBeauty");
                        case Custom1 :
                                return this->setRadiusName("Custom1");
                        case Custom2 :
                                return this->setRadiusName("Custom2");
                        case Custom3 :
                                return this->setRadiusName("Custom3");
                        case Custom4 :
                                return this->setRadiusName("Custom4");
                }   

}

TString LauBlattWeisskopfFactor::setRadiusName( const TString& categoryName )
{
        TString name = "BarrierRadius_";
        name.Append(categoryName);

        radius_->name(name);

        return categoryName;
}
tlatham: Apologies, I accidentally omitted a few details. To avoid further ambiguities, here's exactly…
johndanAuthorUnsubmitted
Done
Inline Actions

Looks good. The compiler doesn't like the switch with no return outside:

/afs/cern.ch/user/j/johndan/ExternalSoftware/Laura++/laura/src/LauBlattWeisskopfFactor.cc: In member function 'TString LauBlattWeisskopfFactor::setRadiusName(LauBlattWeisskopfFactor::BlattWeisskopfCategory)':
/afs/cern.ch/user/j/johndan/ExternalSoftware/Laura++/laura/src/LauBlattWeisskopfFactor.cc:124:1: warning: control reaches end of non-void function [-Wreturn-type]
  124 | }
johndan: Looks good. The compiler doesn't like the switch with no return outside: ``` /afs/cern.
tlathamUnsubmitted
Done
Inline Actions

Oh yes, I'd forgotten about gcc's insistence that control can get beyond a switch on an enum that covers every case and returns from each (clang is happy that it can't).
I'd just add:

// We should never get here but gcc seems to think we can
return this->setRadiusName("Unknown");

at the end.

tlatham: Oh yes, I'd forgotten about gcc's insistence that control can get beyond a switch on an enum…
return this->setRadiusName( category, resInfo.getSanitisedName() );
break;
default :
return this->setRadiusName( category );
break;
}
}
TString LauBlattWeisskopfFactor::setRadiusName( const BlattWeisskopfCategory category, TString specifiedCategoryName )
{
TString name = "BarrierRadius_"; TString name = "BarrierRadius_";
TString categoryName; TString categoryName;
if ( specifiedCategoryName.EqualTo("") ) {
switch (category) { switch (category) {
case Parent : case Parent :
categoryName = "Parent"; categoryName = "Parent";
break; break;
case Indep :
categoryName = resInfo.getSanitisedName();
break;
case Light : case Light :
categoryName = "Light"; categoryName = "Light";
break; break;
case Kstar : case Kstar :
categoryName = "Kstar"; categoryName = "Kstar";
break; break;
case Charm : case Charm :
categoryName = "Charm"; categoryName = "Charm";
break; break;
case StrangeCharm : case StrangeCharm :
categoryName = "StrangeCharm"; categoryName = "StrangeCharm";
break; break;
case Charmonium : case Charmonium :
categoryName = "Charmonium"; categoryName = "Charmonium";
break; break;
case Beauty : case Beauty :
categoryName = "Beauty"; categoryName = "Beauty";
break; break;
case StrangeBeauty : case StrangeBeauty :
categoryName = "StrangeBeauty"; categoryName = "StrangeBeauty";
break; break;
case CharmBeauty : case CharmBeauty :
categoryName = "CharmBeauty"; categoryName = "CharmBeauty";
break; break;
case Custom1 : case Custom1 :
categoryName = "Custom1"; categoryName = "Custom1";
break; break;
case Custom2 : case Custom2 :
categoryName = "Custom2"; categoryName = "Custom2";
break; break;
case Custom3 : case Custom3 :
categoryName = "Custom3"; categoryName = "Custom3";
break; break;
case Custom4 : case Custom4 :
categoryName = "Custom4"; categoryName = "Custom4";
break; break;
default : default :
categoryName = "Unknown"; categoryName = "Unknown";
johndanAuthorUnsubmitted
Done
Inline Actions

I'm amazed that C++ doesn't have a mechanism to allow enum types to be converted to strings easily. I saw boost has a preprocessor library that can handle it, but managing it in C++ looks messy in the examples I've seen.

johndan: I'm amazed that C++ doesn't have a mechanism to allow enum types to be converted to strings…
tlathamUnsubmitted
Done
Inline Actions

Indeed, it is a bit surprising that this isn't easier to do. There are some clever tricks that can be done with macros to autogenerate this stuff but they're hard to read, so I prefer to keep it like this, at least for now.

tlatham: Indeed, it is a bit surprising that this isn't easier to do. There are some clever tricks that…
johndanAuthorUnsubmitted
Done
Inline Actions

Yes, I entirely agree. The boost solution was the least messy of them all, but certainly not worth introducing dependencies to achieve this minor goal.

johndan: Yes, I entirely agree. The boost solution was the least messy of them all, but certainly not…
break; break;
} }
}
else {
categoryName = specifiedCategoryName;
}
name.Append(categoryName); name.Append(categoryName);
radius_->name(name); radius_->name(name);
return categoryName; return categoryName;
} }
LauBlattWeisskopfFactor* LauBlattWeisskopfFactor::createClone( const UInt_t newSpin, const BarrierType newBarrierType ) LauBlattWeisskopfFactor* LauBlattWeisskopfFactor::createClone( const UInt_t newSpin, const BarrierType newBarrierType )
▲ Show 20 Lines • Show All 60 Lines • Show Last 20 Lines