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

Poco::Logger can not output __FILE__ and __LINE__ #4553

Closed
siren186 opened this issue May 11, 2024 · 12 comments · Fixed by #4555
Closed

Poco::Logger can not output __FILE__ and __LINE__ #4553

siren186 opened this issue May 11, 2024 · 12 comments · Fixed by #4555

Comments

@siren186
Copy link

siren186 commented May 11, 2024

Describe the bug
Poco::Logger can not output __FILE__ and __LINE__

To Reproduce

// Here is the problem, It's very strange in my Visual Studio 2019 (with Qt5.15.2)
// I don't know why, The name should be "int", But in fact, name == "long"
std::string name = typeid(__LINE__).name();

poco_information(logger(), "hello"); // Error !!! Log has no __FILE__ and __LINE__
logger().information("hello", __FILE__, __LINE__); // Error !!! Log has no __FILE__ and __LINE__
logger().information("hello", __FILE__, (int)__LINE__); // OK

If the type of __LINE__ is int, Poco::Logger will call this method:

void information(const std::string& msg, const char* file, int line);

But if the type of __LINE__ is long, Poco::Logger will call this method:

template <typename T, typename... Args>
void information(const std::string& fmt, T arg1, Args&&... args)
{
if (information())
	logAlways(Poco::format(fmt, arg1, std::forward<Args>(args)...), Message::PRIO_INFORMATION);
}

Then it will lost __FILE__ and __LINE__.

Please add relevant environment information:

  • Window 10
  • poco-1.13.3
  • Visual Studio 2019
@siren186 siren186 added the bug label May 11, 2024
@andrewauclair
Copy link
Contributor

Are you running into this issue with /ZI in Debug? #2913 (comment)

@siren186
Copy link
Author

siren186 commented May 13, 2024

Are you running into this issue with /ZI in Debug? #2913 (comment)

Yes, you are right !
What about add some methods like

void information(const std::string& msg, const char* file, int line);
void information(const std::string& msg, const char* file, long int line); // new method to add

to fix the problem ?

@andrewauclair
Copy link
Contributor

That would be a breaking change to any existing calls to information that use those types. The only real option that comes to mind is to use static_cast<int>(__LINE__) instead of just __LINE__ in the logger macros.

@bas524
Copy link
Contributor

bas524 commented May 13, 2024

Hi!
What do you think about replacing method

void information(const std::string& msg, const char* file, int line);

with

template<typename T, std::enable_if_t<std::is_integral<T>::value, bool> = true>
void information(const std::string& msg, const char* file, T line);

?
IMO this isn't breaking change

@aleks-f aleks-f added enhancement and removed bug labels May 13, 2024
@aleks-f
Copy link
Member

aleks-f commented May 13, 2024

@bas524 that looks like it should work

@aleks-f aleks-f added this to the Release 1.14.0 milestone May 13, 2024
@bas524
Copy link
Contributor

bas524 commented May 13, 2024

@siren186, can you help me to set correct compiler options? I want to verify my changes in godbolt, but I can't get described behavour when __LINE__ is long
link with sample code

@andrewauclair
Copy link
Contributor

andrewauclair commented May 13, 2024

IMO this isn't breaking change

Opinions aside, this is a breaking change. Using a template doesn't change the output. The same thing would happen with just void information(const std::string& msg, const char* file, long line). See this godbolt for a comparison: https://godbolt.org/z/v9hGsjd8s.

Any calls to the parameter pack version of information such as information("%s:%d", "127.0.0.1", 5000L) would be broken.

@bas524
Copy link
Contributor

bas524 commented May 13, 2024

@andrewauclair , I think that I found another good way to resolve the problem without templates
I fix type of __LINE__ with decltype and use this type for declaration

namespace Poco {

using LineNumber = decltype(__LINE__);

}

and then

void log(const std::string& text, Message::Priority prio, const char* file, LineNumber line);

@andrewauclair
Copy link
Contributor

That would cause the same issue. Although it would only cause the issue for when the type of __LINE__ is long. I'm also not sure that would actually work if Poco was compiled without ZI but the application was.

The ultimate (but sadly still breaking) change would be to separate the log functions that takes file and line from the other log functions. I've wanted to add some optional C++20 logging with std::source_location for a while as well.

@siren186
Copy link
Author

@siren186, can you help me to set correct compiler options? I want to verify my changes in godbolt, but I can't get described behavour when __LINE__ is long link with sample code

It compiled with errors.

@andrewauclair , I think that I found another good way to resolve the problem without templates I fix type of __LINE__ with decltype and use this type for declaration

namespace Poco {

using LineNumber = decltype(__LINE__);

}

and then

void log(const std::string& text, Message::Priority prio, const char* file, LineNumber line);

It works well !!!

@matejk
Copy link
Contributor

matejk commented May 13, 2024

@andrewauclair , I think that I found another good way to resolve the problem without templates I fix type of __LINE__ with decltype and use this type for declaration

namespace Poco {

using LineNumber = decltype(__LINE__);

}

and then

void log(const std::string& text, Message::Priority prio, const char* file, LineNumber line);

IMO this is the most elegant solution because the type for the line number is derived from the compiler/platform.

@andrewauclair
Copy link
Contributor

Here's what I think would be a good compromise. A breaking change is required to properly account for long __LINE__ and the current logging functions that log file and line number interfere with the variadic template versions of the functions. It would be good to separate these into different functions. To make sure the breaking change is clear, we can mark the existing log function with file and line as deleted. Code then can be migrated to the new logWithLocation and the deleted version can be removed after a release or two. This would allow using the variadic template form with those arguments.

As a bonus, we can define a version of logWithVersion that uses the new std::source_location if the header is available. The actual check for this include would be a lot messier than this, but this is just an example. This can technically be done with the variadic template form as well using deduction guides to allow the proper calling of the function.

Godbolt for this code: https://godbolt.org/z/rM1T3d5qa

#include <iostream>
#include <string>
#include <format>

#if __has_include(<source_location>)
#include <source_location>
#endif

struct Logger {
    // delete the existing log function with file and line to make the breaking change obvious
    void log(const std::string& msg, const char* file, int line) = delete;

    // add a new logging function with a different name to prevent confusion with the variadic template version
    void logWithLocation(const std::string& msg, const char* file, decltype(__LINE__) line) {
        std::cout << file << ":" << line << ": " << msg << '\n';
    }

    // new log function with location using the C++20 std::source_location for information
#if __has_include(<source_location>)
    void logWithLocation(const std::string& msg, std::source_location location = std::source_location::current()) {
        std::cout << location.file_name() << '('
            << location.line() << ':'
            << location.column() << ") `"
            << location.function_name() << ": " << msg << '\n';
    }
#endif

    template<typename T, typename... Args>
    void log (const std::string& format, T t, Args... args) {
        std::cout << std::vformat(format, std::make_format_args(t, args...)) << '\n';
    }
};

int main()
{
    Logger logger;

    logger.logWithLocation("failed here", __FILE__, __LINE__);
    logger.logWithLocation("failed again");
    logger.log("{}:{}", "127.0.0.1", 5000L);
}
/app/example.cpp:38: failed here
/app/example.cpp(39:27) `int main(): failed again
127.0.0.1:5000

bas524 added a commit to bas524/poco that referenced this issue May 13, 2024
MS Visual Studio can use type long for __LINE__ macro when /ZI
compilation flag is used - https://learn.microsoft.com/en-us/cpp/build/
reference/z7-zi-zi-debug-information-format?view=msvc-170#zi-1
This breaks some poco interfaces, for ex. logger
We should fix type for line number
@bas524 bas524 mentioned this issue May 13, 2024
matejk pushed a commit that referenced this issue May 15, 2024
MS Visual Studio can use type long for __LINE__ macro when /ZI
compilation flag is used - https://learn.microsoft.com/en-us/cpp/build/
reference/z7-zi-zi-debug-information-format?view=msvc-170#zi-1
This breaks some poco interfaces, for ex. logger
We should fix type for line number
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

5 participants