Skip to content

Following changes in most recent FairLogger tag using string_view instead of string#95

Closed
ehellbar wants to merge 1 commit intoAliceO2Group:masterfrom
ehellbar:master
Closed

Following changes in most recent FairLogger tag using string_view instead of string#95
ehellbar wants to merge 1 commit intoAliceO2Group:masterfrom
ehellbar:master

Conversation

@ehellbar
Copy link

@ehellbar ehellbar commented Feb 11, 2025

FairLogger v2.1.0 introduces a new ("critical") severity to be used in O2. The previous FairLogger used was v1.11.1, so there were further changes in the meantime. One of them is changing std::string members to std::string_view members in https://github.com/FairRootGroup/FairLogger/blob/27527ad87b63213b7c58caf1e34a538e76f273da/logger/Logger.h#L176

This would also require a new InfoLogger tag, to be bumped together with FairLogger in alisw/alidist#5763

AliceO2::InfoLogger::InfoLogger::undefinedMessageOption.errorCode,
metadata.file.c_str(),
atoi(metadata.line.c_str())
std::string(metadata.file.data(), metadata.file.size()).c_str(),
Copy link
Member

Choose a reason for hiding this comment

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

Does InfoLoggerMessageOption copy the string?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No copy, InfoLoggerMessageOption is a POD with a char*. It's meant to be resolved at compile time whenever possible.

Copy link
Author

Choose a reason for hiding this comment

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

I replaced the char* in InfoLoggerMessageOption with a string_view, plus subsequent changes

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hello @ehellbar,
thanks for the heads-up on the FMQ interface changes.
I prefer to avoid changes in the internal infoLogger structures, and limit the update to the FMQ external support file.

InfoLogger v2.8.0 now works fine with FMQLogger v2.1.0
940fefa

Copy link
Author

@ehellbar ehellbar Feb 18, 2025

Choose a reason for hiding this comment

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

@sy-c thanks for taking care of it.

simply creating the string here 940fefa#diff-36584f894efde34aee0e3aedfd701712f9a7767d6bd748c11dc604bfd937d9acL94 was also my first thought. But @ktf reminded me that std::string(metadata.file) will go out of scope and the const char* of AliceO2::InfoLogger::InfoLogger::InfoLoggerMessageOption opt will point to something undefined. Or am I missing something different?

Copy link
Collaborator

@sy-c sy-c Feb 19, 2025

Choose a reason for hiding this comment

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

You are right, sorry for this. Please use v2.8.1. #97

Valgrind did not complained in my tests, I guess the compiler optimized it as a singleton in the log() call - which is how it is usually written: InfoLoggerMessageOption directly set in the log() call without defining a variable. Here with the IF and two log() we indeed need the intermediate step.

Copy link
Author

Choose a reason for hiding this comment

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

I guess the compiler optimized it as a singleton in the log() call - which is how it is usually written: InfoLoggerMessageOption directly set in the log() call without defining a variable.

I see, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants