Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updated TRestMetadata::GetFieldValue to allow for default value #277

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

lobis
Copy link
Member

@lobis lobis commented Jul 29, 2022

lobis Ok: 9

This PR was made to allow TRestMetadata::GetFieldValue to have a default value, instead of returning "Not defined".

However I could not make it work, there is some problem related to reflection I could not workout, not even creating an additional function GetFieldValueWithDefault that did this task...

I think this is necessary in order to avoid patterns like

bool isSensitive = false;
const string isSensitiveValue = GetFieldValue("sensitive", volumeDefinition);
if (isSensitiveValue != "Not defined") {
    isSensitive = StringToBool(isSensitiveValue);
}

which could be replaced by

const bool isSensitive = StringToBool(GetFieldValue("sensitive", volumeDefinition, "false"));

Could anyone (probably @nkx111) @rest-for-physics/core_dev take a look please? or perhaps there is an alternative (clean) way to achieve the same? Thanks

@lobis lobis requested a review from nkx111 July 29, 2022 19:41
@lobis lobis added the help wanted Extra attention is needed label Jul 29, 2022
@lobis lobis marked this pull request as ready for review July 29, 2022 19:47
@jgalan
Copy link
Member

jgalan commented Sep 22, 2022

It looks as the compilation error is related with Bool_t and bool types? Perhaps the const is causing problems here?

/builds/rest-for-physics/framework/source/framework/tools/inc/TRestReflector.h:471:84: error: invalid conversion from 'Bool_t (*)(const string&)' {aka 'bool (*)(const std::__cxx11::basic_string<char>&)'} to 'bool (*)(std::string)' {aka 'bool (*)(std::__cxx11::basic_string<char>)'} [-fpermissive]
[388](https://gitlab.cern.ch/rest-for-physics/framework/-/jobs/23650145#L388)

@@ -36,7 +36,7 @@ Double_t StringToDouble(std::string in);
Int_t StringToInteger(std::string in);
std::string IntegerToString(Int_t n, std::string format = "%d");
std::string DoubleToString(Double_t d, std::string format = "%4.2lf");
Bool_t StringToBool(std::string in);
Bool_t StringToBool(const std::string& in);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The compilation failure is due to this const keyword. It makes the argument unmatched. If we think this change is necessary then we shall open a new PR to replace all the AddConverter(xxx) with const-input methods.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be implemented if it's not too much work, its not high priority but using const T& should always be used to pass non built-in types or atleast be the default option, so I think we should support it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants