Re: [log4cxx] Short filename options
Thinking about this a bit more still, I think we can do this in two ways to make everything work properly. We can check for the __cpp_lib_string_view macro and/or _MSVC_LANG(because windows always needs to be different), and if the compiler supports std::string view we use that at compile-time to pass in the const char* to the LocationInfo class. If we can't do it at compile time, we will figure it out at runtime inside of the LocationInfo. Either way this adds one const char* to the LocationInfo class. Regardless, the full path to the file is still embedded in the binaries of users of the library unless you explicitly turn that off. I'll add some documentation related to that. -Robert Middleton On Sun, Jan 2, 2022 at 8:07 AM Thorsten Schöning wrote: > > Guten Tag Robert Middleton, > am Sonntag, 2. Januar 2022 um 02:38 schrieben Sie: > > > 2. Create a constexpr function that we control[...] > > 3. Use std::string_view(for C++17) or > > boost::string_view(pre-c++17).[...] > > Just to make sure I understand correctly: The difference between 2 and > 3 is using a custom implementation vs. an already available one? > > As we rely on boost for whatever is not supported by the compiler > anyway already, I prefer yur option 3 then. Looking at the PR, we had > already three different implementations proposed in the end and I > don't see any benefit to discuss those further when "string_view" > handles all of this already. > > > [...]I can see certain > > circumstances where it is useful to have the full path, for example > > when you have two files named the same(please don't do this). > > I'm doing this sometimes and in theory it shouldn't be a problem if > placed into different namespaces. Shouldn't it? Of course some broken > IDEs/compilers like Embarcadero C++-Builder don't work properly with > that in case of incremental builds, but that's not too much of a > problem for me. > > Regarding absolute paths, one might want to keep in mind that not > everyone ships results from automatic builds like Jenkins only and/or > uses multiple different branches for different customers with slightly > modified codebase at the same time. So keeping absolute paths if > available before makes sense to me. > > Mit freundlichen Grüßen > > Thorsten Schöning > > -- > AM-SoFT IT-Service - Bitstore Hameln GmbH > Mitglied der Bitstore Gruppe - Ihr Full-Service-Dienstleister für IT und TK > > E-Mail: thorsten.schoen...@am-soft.de > Web:http://www.AM-SoFT.de/ > > Tel: 05151- 9468- 0 > Tel: 05151- 9468-55 > Fax: 05151- 9468-88 > Mobil: 0178-8 9468-04 > > AM-SoFT IT-Service - Bitstore Hameln GmbH, Brandenburger Str. 7c, 31789 Hameln > AG Hannover HRB 221853 - Geschäftsführer: Janine Galonska > > > >
Re: [log4cxx] Short filename options
Guten Tag Robert Middleton, am Sonntag, 2. Januar 2022 um 02:38 schrieben Sie: > 2. Create a constexpr function that we control[...] > 3. Use std::string_view(for C++17) or > boost::string_view(pre-c++17).[...] Just to make sure I understand correctly: The difference between 2 and 3 is using a custom implementation vs. an already available one? As we rely on boost for whatever is not supported by the compiler anyway already, I prefer yur option 3 then. Looking at the PR, we had already three different implementations proposed in the end and I don't see any benefit to discuss those further when "string_view" handles all of this already. > [...]I can see certain > circumstances where it is useful to have the full path, for example > when you have two files named the same(please don't do this). I'm doing this sometimes and in theory it shouldn't be a problem if placed into different namespaces. Shouldn't it? Of course some broken IDEs/compilers like Embarcadero C++-Builder don't work properly with that in case of incremental builds, but that's not too much of a problem for me. Regarding absolute paths, one might want to keep in mind that not everyone ships results from automatic builds like Jenkins only and/or uses multiple different branches for different customers with slightly modified codebase at the same time. So keeping absolute paths if available before makes sense to me. Mit freundlichen Grüßen Thorsten Schöning -- AM-SoFT IT-Service - Bitstore Hameln GmbH Mitglied der Bitstore Gruppe - Ihr Full-Service-Dienstleister für IT und TK E-Mail: thorsten.schoen...@am-soft.de Web:http://www.AM-SoFT.de/ Tel: 05151- 9468- 0 Tel: 05151- 9468-55 Fax: 05151- 9468-88 Mobil: 0178-8 9468-04 AM-SoFT IT-Service - Bitstore Hameln GmbH, Brandenburger Str. 7c, 31789 Hameln AG Hannover HRB 221853 - Geschäftsführer: Janine Galonska
Re: [log4cxx] Short filename options
On Sat, Jan 01, 2022 at 08:38:43PM -0500, Robert Middleton wrote: > The full path is already in the compiled code anyway, this would > simply add the ability to get the filename from the full path(so > another member to the LocationInfo class). I can see certain > circumstances where it is useful to have the full path, for example > when you have two files named the same(please don't do this). > > That does bring up a good point though, is that if you don't want > location info compiled in, we should have a preprocessor macro that > will compile out all of the location info data. We can already > compile out log messages of certain levels, so hiding the location is > just a natural extension of that. Current log4cxx is reproducible [1] as in reproducible-builds.org, and one part of checking reproducibilty is to build in different location, it looks like that current log4cxx does NOT embed the complete path. So a wish from the Debian mainainer: It would be nice to retain reproduciblity when implementing this feature... A happy new year! -- tobi [1] https://tests.reproducible-builds.org/debian/rb-pkg/unstable/amd64/log4cxx.html (0.12.1 is currently not reproducible due to something doxygen is doing, but the library package is.) > -Robert Middleton > > On Sat, Jan 1, 2022 at 7:50 PM Stephen Webb wrote: > > > > I prefer option 2 if it is possible. > > > > I do not think it is useful to have the full path name (of the build > > directory) in shipped binaries. > > > > On Sun, Jan 2, 2022 at 7:11 AM Robert Middleton > > wrote: > > > > > PR #75 adds a new option for displaying the short filename of the log > > > location, which is probably a good idea to have, as in my experience, > > > the whole path of the file is not that useful, especially when the > > > binary is from a build server where the path is something like > > > /var/lib/jenkins/project-name/src/main/directory/foo.cpp. > > > > > > The current PR does this with some string manipulation at runtime, and > > > is different from the const char* that is currently used, so it > > > doesn't fit that well with the rest of the class. Since C++11 we can > > > now do constexpr functions, so we should be able to do this at compile > > > time(assuming you have optimizations turned on of course). > > > > > > We can do this one of several ways: > > > 1. Take the PR as is(use strings and calculate at runtime) > > > 2. Create a constexpr function that we control to calculate the > > > filename at compile time as either an offset into the filename or a > > > separate const char*. The advantage to this is that it doesn't > > > require any other libraries for pre-C++17 compilers. > > > 3. Use std::string_view(for C++17) or boost::string_view(pre-c++17). > > > The following would work for this solution: > > > std::string_view str{"/foo/bar/what.cpp"}; > > > const int location = str.find_last_of( '/' ) + 1; > > > printf( "fullPath: %s\n", str.data() ); > > > printf( "fileName: %s\n", &str[location] ); > > > > > > Does anybody have a preference or a better way to do it? I'm inclined > > > to go with (3), since that does provide a good fallback for compilers > > > that don't support C++17, and only requires boost for older compilers. > > > > > > -Robert Middleton > > >
Re: [log4cxx] Short filename options
This is already provided by ``` #define LOG4CXX_LOCATION ::log4cxx::spi::LocationInfo() #include ``` On Sun, Jan 2, 2022 at 12:38 PM Robert Middleton wrote: > The full path is already in the compiled code anyway, this would > simply add the ability to get the filename from the full path(so > another member to the LocationInfo class). I can see certain > circumstances where it is useful to have the full path, for example > when you have two files named the same(please don't do this). > > That does bring up a good point though, is that if you don't want > location info compiled in, we should have a preprocessor macro that > will compile out all of the location info data. We can already > compile out log messages of certain levels, so hiding the location is > just a natural extension of that. > > -Robert Middleton > > On Sat, Jan 1, 2022 at 7:50 PM Stephen Webb wrote: > > > > I prefer option 2 if it is possible. > > > > I do not think it is useful to have the full path name (of the build > > directory) in shipped binaries. > > > > On Sun, Jan 2, 2022 at 7:11 AM Robert Middleton > > wrote: > > > > > PR #75 adds a new option for displaying the short filename of the log > > > location, which is probably a good idea to have, as in my experience, > > > the whole path of the file is not that useful, especially when the > > > binary is from a build server where the path is something like > > > /var/lib/jenkins/project-name/src/main/directory/foo.cpp. > > > > > > The current PR does this with some string manipulation at runtime, and > > > is different from the const char* that is currently used, so it > > > doesn't fit that well with the rest of the class. Since C++11 we can > > > now do constexpr functions, so we should be able to do this at compile > > > time(assuming you have optimizations turned on of course). > > > > > > We can do this one of several ways: > > > 1. Take the PR as is(use strings and calculate at runtime) > > > 2. Create a constexpr function that we control to calculate the > > > filename at compile time as either an offset into the filename or a > > > separate const char*. The advantage to this is that it doesn't > > > require any other libraries for pre-C++17 compilers. > > > 3. Use std::string_view(for C++17) or boost::string_view(pre-c++17). > > > The following would work for this solution: > > > std::string_view str{"/foo/bar/what.cpp"}; > > > const int location = str.find_last_of( '/' ) + 1; > > > printf( "fullPath: %s\n", str.data() ); > > > printf( "fileName: %s\n", &str[location] ); > > > > > > Does anybody have a preference or a better way to do it? I'm inclined > > > to go with (3), since that does provide a good fallback for compilers > > > that don't support C++17, and only requires boost for older compilers. > > > > > > -Robert Middleton > > > >
Re: [log4cxx] Short filename options
The full path is already in the compiled code anyway, this would simply add the ability to get the filename from the full path(so another member to the LocationInfo class). I can see certain circumstances where it is useful to have the full path, for example when you have two files named the same(please don't do this). That does bring up a good point though, is that if you don't want location info compiled in, we should have a preprocessor macro that will compile out all of the location info data. We can already compile out log messages of certain levels, so hiding the location is just a natural extension of that. -Robert Middleton On Sat, Jan 1, 2022 at 7:50 PM Stephen Webb wrote: > > I prefer option 2 if it is possible. > > I do not think it is useful to have the full path name (of the build > directory) in shipped binaries. > > On Sun, Jan 2, 2022 at 7:11 AM Robert Middleton > wrote: > > > PR #75 adds a new option for displaying the short filename of the log > > location, which is probably a good idea to have, as in my experience, > > the whole path of the file is not that useful, especially when the > > binary is from a build server where the path is something like > > /var/lib/jenkins/project-name/src/main/directory/foo.cpp. > > > > The current PR does this with some string manipulation at runtime, and > > is different from the const char* that is currently used, so it > > doesn't fit that well with the rest of the class. Since C++11 we can > > now do constexpr functions, so we should be able to do this at compile > > time(assuming you have optimizations turned on of course). > > > > We can do this one of several ways: > > 1. Take the PR as is(use strings and calculate at runtime) > > 2. Create a constexpr function that we control to calculate the > > filename at compile time as either an offset into the filename or a > > separate const char*. The advantage to this is that it doesn't > > require any other libraries for pre-C++17 compilers. > > 3. Use std::string_view(for C++17) or boost::string_view(pre-c++17). > > The following would work for this solution: > > std::string_view str{"/foo/bar/what.cpp"}; > > const int location = str.find_last_of( '/' ) + 1; > > printf( "fullPath: %s\n", str.data() ); > > printf( "fileName: %s\n", &str[location] ); > > > > Does anybody have a preference or a better way to do it? I'm inclined > > to go with (3), since that does provide a good fallback for compilers > > that don't support C++17, and only requires boost for older compilers. > > > > -Robert Middleton > >
Re: [log4cxx] Short filename options
I prefer option 2 if it is possible. I do not think it is useful to have the full path name (of the build directory) in shipped binaries. On Sun, Jan 2, 2022 at 7:11 AM Robert Middleton wrote: > PR #75 adds a new option for displaying the short filename of the log > location, which is probably a good idea to have, as in my experience, > the whole path of the file is not that useful, especially when the > binary is from a build server where the path is something like > /var/lib/jenkins/project-name/src/main/directory/foo.cpp. > > The current PR does this with some string manipulation at runtime, and > is different from the const char* that is currently used, so it > doesn't fit that well with the rest of the class. Since C++11 we can > now do constexpr functions, so we should be able to do this at compile > time(assuming you have optimizations turned on of course). > > We can do this one of several ways: > 1. Take the PR as is(use strings and calculate at runtime) > 2. Create a constexpr function that we control to calculate the > filename at compile time as either an offset into the filename or a > separate const char*. The advantage to this is that it doesn't > require any other libraries for pre-C++17 compilers. > 3. Use std::string_view(for C++17) or boost::string_view(pre-c++17). > The following would work for this solution: > std::string_view str{"/foo/bar/what.cpp"}; > const int location = str.find_last_of( '/' ) + 1; > printf( "fullPath: %s\n", str.data() ); > printf( "fileName: %s\n", &str[location] ); > > Does anybody have a preference or a better way to do it? I'm inclined > to go with (3), since that does provide a good fallback for compilers > that don't support C++17, and only requires boost for older compilers. > > -Robert Middleton >
[log4cxx] Short filename options
PR #75 adds a new option for displaying the short filename of the log location, which is probably a good idea to have, as in my experience, the whole path of the file is not that useful, especially when the binary is from a build server where the path is something like /var/lib/jenkins/project-name/src/main/directory/foo.cpp. The current PR does this with some string manipulation at runtime, and is different from the const char* that is currently used, so it doesn't fit that well with the rest of the class. Since C++11 we can now do constexpr functions, so we should be able to do this at compile time(assuming you have optimizations turned on of course). We can do this one of several ways: 1. Take the PR as is(use strings and calculate at runtime) 2. Create a constexpr function that we control to calculate the filename at compile time as either an offset into the filename or a separate const char*. The advantage to this is that it doesn't require any other libraries for pre-C++17 compilers. 3. Use std::string_view(for C++17) or boost::string_view(pre-c++17). The following would work for this solution: std::string_view str{"/foo/bar/what.cpp"}; const int location = str.find_last_of( '/' ) + 1; printf( "fullPath: %s\n", str.data() ); printf( "fileName: %s\n", &str[location] ); Does anybody have a preference or a better way to do it? I'm inclined to go with (3), since that does provide a good fallback for compilers that don't support C++17, and only requires boost for older compilers. -Robert Middleton