-
Notifications
You must be signed in to change notification settings - Fork 271
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
C++17: sym link to latest log #366
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting!
It’s about time to lift G3log up to C++17 support so I can take a look at this in light of that coming work. I would have to make a last C++14 release and branch first (similar to what I did for C++11).
Have you looked at g3sinks repo?
I’m surprised you got it to compile though with C++17. I thought that the handling of result_of
(deprecated) would cause some issues. Ref: https://en.cppreference.com/w/cpp/types/result_of
Lol. It’s early morning here. Of course you have looked at g3sinks ;) |
I just modified the cmake |
Please don't close this PR. I will get to it but it'll take a bit of time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xgdgsc sorry for the late action on this. Can you update the PR now that g3log is c++17 compatible? There are no reasons for the ifdef anymore
@@ -10,6 +10,7 @@ | |||
#include "filesinkhelper.ipp" | |||
#include <cassert> | |||
#include <chrono> | |||
#include <filesystem> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dang, apparently c++-17 still has the experimental/filesystem
on gcc (Linux/OSX). I think that's why it's failing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use this snippet of code to make getting to std::filesystem work on a variety of compilers/platforms. Not exhaustive, of course.
#include <filesystem> | |
#if defined(_WIN32) | |
# if _MSC_VER >= 1914 | |
# include <filesystem> | |
namespace fs = std::filesystem; | |
# else | |
# include <experimental/filesystem> | |
namespace fs = std::experimental::filesystem; | |
# endif | |
#elif defined(__GNUC__) && __GNUC__ <= 7 | |
# include <experimental/filesystem> | |
namespace fs = std::experimental::filesystem; | |
#elif defined(__GNUC__) && __GNUC__ >= 8 | |
# include <filesystem> | |
namespace fs = std::filesystem; | |
#elif defined(__clang__) && __clang_major__ <= 8 | |
# include <experimental/filesystem> | |
namespace fs = std::experimental::filesystem; | |
#elif defined(__clang__) && __clang_major__ <= 10 | |
# include <filesystem> | |
namespace fs = std::filesystem; | |
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this could be used?
Not sure on Windows but I think it works on Linux and OSX
// since C++ 20
#include <version>
#ifdef __cpp_lib_filesystem
#include <filesystem>
using fs = std::filesystem;
#elif __cpp_lib_experimental_filesystem
#include <experimental/filesystem>
using fs = std::experimental::filesystem;
#else
#error "no filesystem support ='("
#endif
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think having just made the effort get onto c++17 it makes more sense to start using it.
Tried it out here: #439 |
I find it handy that glog creates symlink to latest log file. This patch requires c++17. So it may not be ok to merge but I think it might help people needing this feature searching in PRs.