You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by GitBox <gi...@apache.org> on 2022/01/02 18:58:04 UTC
[GitHub] [logging-log4cxx] rm5248 opened a new pull request #95: Add short filename to location info
rm5248 opened a new pull request #95:
URL: https://github.com/apache/logging-log4cxx/pull/95
This is an extension of PR #75. This will calculate the short filename at compile time(assuming you have C++17) and give it to the LocationInfo class. If you do not have C++17 and you request the short filename, it will calculate it at runtime.
Documentation on how to disable location information has also been added(with a test).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] ams-tschoening commented on a change in pull request #95: Add short filename to location info
Posted by GitBox <gi...@apache.org>.
ams-tschoening commented on a change in pull request #95:
URL: https://github.com/apache/logging-log4cxx/pull/95#discussion_r778642762
##########
File path: src/main/include/log4cxx/spi/location/locationinfo.h
##########
@@ -45,21 +58,28 @@ class LOG4CXX_EXPORT LocationInfo
static const LocationInfo& getLocationUnavailable();
-
+#ifdef LOG4CXX_HAS_STRING_VIEW
+ static constexpr const char* calcShortFileName(const char* fileName){
+ std::string_view view(fileName);
+ // If the separator is not found, rfind will return -1. Adding 1 to
+ // that will have it pointing at fileName, which is a good fallback.
+ return fileName + view.rfind(LOG4CXX_SHORT_FILENAME_SPLIT_CHAR) + 1;
+ }
+#else
+ static const char* calcShortFileName(const char* fileName){
+ return strrchr(fileName, LOG4CXX_SHORT_FILENAME_SPLIT_CHAR) + 1;
Review comment:
`strrchr` might return a NULL pointer in case SPLIT_CHAR is not found [according to the docs](https://www.cplusplus.com/reference/cstring/strrchr/?kw=strrchr). Shouldn't that be handled? You explicitly mention that case for `string_view`. `NULL + 1` seems wrong.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] ams-tschoening commented on a change in pull request #95: Add short filename to location info
Posted by GitBox <gi...@apache.org>.
ams-tschoening commented on a change in pull request #95:
URL: https://github.com/apache/logging-log4cxx/pull/95#discussion_r778637752
##########
File path: src/main/include/log4cxx/spi/location/locationinfo.h
##########
@@ -45,21 +58,28 @@ class LOG4CXX_EXPORT LocationInfo
static const LocationInfo& getLocationUnavailable();
-
+#ifdef LOG4CXX_HAS_STRING_VIEW
+ static constexpr const char* calcShortFileName(const char* fileName){
+ std::string_view view(fileName);
+ // If the separator is not found, rfind will return -1. Adding 1 to
+ // that will have it pointing at fileName, which is a good fallback.
+ return fileName + view.rfind(LOG4CXX_SHORT_FILENAME_SPLIT_CHAR) + 1;
+ }
+#else
+ static const char* calcShortFileName(const char* fileName){
Review comment:
Some additional docs of why `constexpr` is not possible with `strrchr`:
https://stackoverflow.com/questions/42352114/why-all-function-in-cstring-must-not-have-constexpr
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] ams-tschoening commented on a change in pull request #95: Add short filename to location info
Posted by GitBox <gi...@apache.org>.
ams-tschoening commented on a change in pull request #95:
URL: https://github.com/apache/logging-log4cxx/pull/95#discussion_r777873523
##########
File path: src/main/cpp/locationinfo.cpp
##########
@@ -45,16 +45,35 @@ LocationInfo::LocationInfo( const char* const fileName1,
int lineNumber1 )
: lineNumber( lineNumber1 ),
fileName( fileName1 ),
+ shortFileName(nullptr),
methodName( methodName1 )
{
}
+LocationInfo::LocationInfo( const char* const fileName1,
+ size_t shortFilenameOffset,
+ const char* const methodName1,
+ int lineNumber1 )
+ : lineNumber( lineNumber1 ),
+ fileName( fileName1 ),
+ methodName( methodName1 )
+{
+ if( shortFilenameOffset < 0 || shortFilenameOffset > 300 ){
Review comment:
If the file name calculation works, there won't be such access. If it doesn't work, it could easily happen at 199 chars as well already. OTOH, arbitrary path restrictions regarding their length, allowed characters like spaces etc. are a continuing annoyance for decades, so in my opinion we shouldn't add one unless absolutely necessary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] rm5248 commented on a change in pull request #95: Add short filename to location info
Posted by GitBox <gi...@apache.org>.
rm5248 commented on a change in pull request #95:
URL: https://github.com/apache/logging-log4cxx/pull/95#discussion_r778442479
##########
File path: src/main/include/log4cxx/spi/location/locationinfo.h
##########
@@ -132,9 +152,28 @@ class LOG4CXX_EXPORT LocationInfo
#if !defined(__LOG4CXX_FUNC__)
#define __LOG4CXX_FUNC__ ""
#endif
+
+#if __cpp_lib_string_view || (_MSVC_LANG >= 201703L)
+#include <string_view>
+
+#define LOG4CXX_LOCATION_CREATE ::std::string_view file_name{__FILE__};\
Review comment:
Yes, I like your idea of a separate function, that makes it much cleaner. I was trying to avoid expanding __FILE__ multiple times, until I remembered that the compiler will only put in the same string once into the binary. That also makes it easier to support both versions: one using `string_view` and the fallback using `strrchr`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] rm5248 commented on a change in pull request #95: Add short filename to location info
Posted by GitBox <gi...@apache.org>.
rm5248 commented on a change in pull request #95:
URL: https://github.com/apache/logging-log4cxx/pull/95#discussion_r778442479
##########
File path: src/main/include/log4cxx/spi/location/locationinfo.h
##########
@@ -132,9 +152,28 @@ class LOG4CXX_EXPORT LocationInfo
#if !defined(__LOG4CXX_FUNC__)
#define __LOG4CXX_FUNC__ ""
#endif
+
+#if __cpp_lib_string_view || (_MSVC_LANG >= 201703L)
+#include <string_view>
+
+#define LOG4CXX_LOCATION_CREATE ::std::string_view file_name{__FILE__};\
Review comment:
Yes, I like your idea of a separate function, that makes it much cleaner. I was trying to avoid expanding `__FILE__ ` multiple times, until I remembered that the compiler will only put in the same string once into the binary. That also makes it easier to support both versions: one using `string_view` and the fallback using `strrchr`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] rm5248 merged pull request #95: Add short filename to location info
Posted by GitBox <gi...@apache.org>.
rm5248 merged pull request #95:
URL: https://github.com/apache/logging-log4cxx/pull/95
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] rm5248 commented on a change in pull request #95: Add short filename to location info
Posted by GitBox <gi...@apache.org>.
rm5248 commented on a change in pull request #95:
URL: https://github.com/apache/logging-log4cxx/pull/95#discussion_r777744337
##########
File path: src/main/include/log4cxx/spi/location/locationinfo.h
##########
@@ -132,9 +152,28 @@ class LOG4CXX_EXPORT LocationInfo
#if !defined(__LOG4CXX_FUNC__)
#define __LOG4CXX_FUNC__ ""
#endif
+
+#if __cpp_lib_string_view || (_MSVC_LANG >= 201703L)
+#include <string_view>
+
+#define LOG4CXX_LOCATION_CREATE ::std::string_view file_name{__FILE__};\
Review comment:
Using `string_view` lets it be calculated at compile-time(assuming a high enough optimization level). Upon some further investigation though, both GCC and MSVC can evaluate `strlen` on a static string at compile time. GCC at least is also smart enough to evaluate `strrchr` at compile time, so I think we can do this with only C functions.
Basically the following will work in both MSVC and GCC(at least according to godbolt), with GCC managing to figure out at compile-time that 'name' is '/hithere.cpp':
```
const char* file_name = "/bar/foo/hithere.cpp";
const int file_length = strlen(file_name);
const char* name = strrchr(file_name, '/');
```
Unfortunately it seems that with MSVC using either `string_view` or `strrchr` still requires some processing at run-time in order to calculate the correct position.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] rm5248 commented on a change in pull request #95: Add short filename to location info
Posted by GitBox <gi...@apache.org>.
rm5248 commented on a change in pull request #95:
URL: https://github.com/apache/logging-log4cxx/pull/95#discussion_r779192252
##########
File path: src/main/include/log4cxx/spi/location/locationinfo.h
##########
@@ -45,21 +58,28 @@ class LOG4CXX_EXPORT LocationInfo
static const LocationInfo& getLocationUnavailable();
-
+#ifdef LOG4CXX_HAS_STRING_VIEW
+ static constexpr const char* calcShortFileName(const char* fileName){
+ std::string_view view(fileName);
+ // If the separator is not found, rfind will return -1. Adding 1 to
+ // that will have it pointing at fileName, which is a good fallback.
+ return fileName + view.rfind(LOG4CXX_SHORT_FILENAME_SPLIT_CHAR) + 1;
+ }
+#else
+ static const char* calcShortFileName(const char* fileName){
+ return strrchr(fileName, LOG4CXX_SHORT_FILENAME_SPLIT_CHAR) + 1;
Review comment:
whoops you're right. If it's found we need to add 1(like the string_view version), else we should return the `fileName` as a sane default value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] rm5248 commented on a change in pull request #95: Add short filename to location info
Posted by GitBox <gi...@apache.org>.
rm5248 commented on a change in pull request #95:
URL: https://github.com/apache/logging-log4cxx/pull/95#discussion_r777736412
##########
File path: src/main/cpp/locationinfo.cpp
##########
@@ -45,16 +45,35 @@ LocationInfo::LocationInfo( const char* const fileName1,
int lineNumber1 )
: lineNumber( lineNumber1 ),
fileName( fileName1 ),
+ shortFileName(nullptr),
methodName( methodName1 )
{
}
+LocationInfo::LocationInfo( const char* const fileName1,
+ size_t shortFilenameOffset,
+ const char* const methodName1,
+ int lineNumber1 )
+ : lineNumber( lineNumber1 ),
+ fileName( fileName1 ),
+ methodName( methodName1 )
+{
+ if( shortFilenameOffset < 0 || shortFilenameOffset > 300 ){
Review comment:
My (perhaps misguided) reasoning is that because this accesses a `const char*` directly, that could let you access arbitrary memory. I'm not sure how likely that is though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] ams-tschoening commented on a change in pull request #95: Add short filename to location info
Posted by GitBox <gi...@apache.org>.
ams-tschoening commented on a change in pull request #95:
URL: https://github.com/apache/logging-log4cxx/pull/95#discussion_r777998623
##########
File path: src/main/include/log4cxx/spi/location/locationinfo.h
##########
@@ -132,9 +152,28 @@ class LOG4CXX_EXPORT LocationInfo
#if !defined(__LOG4CXX_FUNC__)
#define __LOG4CXX_FUNC__ ""
#endif
+
+#if __cpp_lib_string_view || (_MSVC_LANG >= 201703L)
+#include <string_view>
+
+#define LOG4CXX_LOCATION_CREATE ::std::string_view file_name{__FILE__};\
Review comment:
So while the macro copies code in the end multiple times, with a supporting compiler the calculation might happen at compile time because of an optimized `string_view`. But that is not guaranteed and the macro might as well be executed multiple times at runtime?
That seems unpredictable and unnecessary confusing to me. Why not use a static function based on `constexpr` instead, which is C++11 and things either reliably break or work? If `string_view` is supported at compile time, shouldn't that be available with `constexpr` then as well? But that would be guaranteed to happen at compile time.
OTOH, your custom implementation might work with `constexpr` as well which is documented to have support for local [variables and loops since C++14](https://en.cppreference.com/w/cpp/language/constexpr). So I suggest something like the following:
```c++
#define LOG4CXX_LOCATION_CREATE ::log4cxx::spi::LocationInfo location(__FILE__, \
::log4cxx::spi::LocationInfo::calcShortFileName(__FILE__), \
__LOG4CXX_FUNC__, \
__LINE__)
```
The implementation of that function checks if `string_view` is available and uses that or your custom implementation. Though, with your custom implementation in backwards, `string_view` is redundant anyway. The functions documents to be calculated at compile time and the macro is less verbose as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] ams-tschoening commented on a change in pull request #95: Add short filename to location info
Posted by GitBox <gi...@apache.org>.
ams-tschoening commented on a change in pull request #95:
URL: https://github.com/apache/logging-log4cxx/pull/95#discussion_r777344076
##########
File path: src/main/cpp/locationinfo.cpp
##########
@@ -45,16 +45,35 @@ LocationInfo::LocationInfo( const char* const fileName1,
int lineNumber1 )
: lineNumber( lineNumber1 ),
fileName( fileName1 ),
+ shortFileName(nullptr),
methodName( methodName1 )
{
}
+LocationInfo::LocationInfo( const char* const fileName1,
+ size_t shortFilenameOffset,
+ const char* const methodName1,
+ int lineNumber1 )
+ : lineNumber( lineNumber1 ),
+ fileName( fileName1 ),
+ methodName( methodName1 )
+{
+ if( shortFilenameOffset < 0 || shortFilenameOffset > 300 ){
Review comment:
What's the reason for the arbitrary limit into to the path? We shouldn't care about how long user paths are, those limits will break for someone.
##########
File path: src/main/cpp/locationinfo.cpp
##########
@@ -102,6 +122,31 @@ const char* LocationInfo::getFileName() const
return fileName;
}
+/**
+ * Return the short file name of the caller.
+ * @returns file name, may be null.
+ */
+const char* LocationInfo::getShortFileName() const{
+ if(shortFileName){
+ return shortFileName;
+ }
+
+ if(fileName == nullptr){
+ return nullptr;
+ }
+
+ const char* begin = fileName;
+ size_t pos = 0;
+ while( fileName[pos] != 0 ){
+ if( fileName[pos] == LOG4CXX_SHORT_FILENAME_SPLIT_CHAR &&
+ fileName[pos+1] != 0 ){
+ begin = fileName + pos + 1;
+ }
+ pos++;
+ }
+ return begin;
Review comment:
Wouldn't it make sense to cache the calculated value using `shortFileName` as well?
##########
File path: src/main/cpp/locationinfo.cpp
##########
@@ -102,6 +122,31 @@ const char* LocationInfo::getFileName() const
return fileName;
}
+/**
+ * Return the short file name of the caller.
+ * @returns file name, may be null.
+ */
+const char* LocationInfo::getShortFileName() const{
+ if(shortFileName){
+ return shortFileName;
+ }
+
+ if(fileName == nullptr){
+ return nullptr;
+ }
+
+ const char* begin = fileName;
+ size_t pos = 0;
Review comment:
Shouldn't that preferrably implement a [backwards search](https://github.com/apache/logging-log4cxx/pull/75#discussion_r757029168) like `rfind` does?
##########
File path: src/main/cpp/locationinfo.cpp
##########
@@ -45,16 +45,35 @@ LocationInfo::LocationInfo( const char* const fileName1,
int lineNumber1 )
: lineNumber( lineNumber1 ),
fileName( fileName1 ),
+ shortFileName(nullptr),
methodName( methodName1 )
{
}
+LocationInfo::LocationInfo( const char* const fileName1,
+ size_t shortFilenameOffset,
Review comment:
`shortFilenameOffset` vs. `shortFileNameOffset`? You seem to have `shortFileName` in all other places.
##########
File path: src/main/cpp/locationinfo.cpp
##########
@@ -45,16 +45,35 @@ LocationInfo::LocationInfo( const char* const fileName1,
int lineNumber1 )
: lineNumber( lineNumber1 ),
fileName( fileName1 ),
+ shortFileName(nullptr),
Review comment:
Shouldn't this use `LocationInfo::NA` like for the default CTOR?
##########
File path: src/main/cpp/locationinfo.cpp
##########
@@ -102,6 +122,31 @@ const char* LocationInfo::getFileName() const
return fileName;
}
+/**
+ * Return the short file name of the caller.
+ * @returns file name, may be null.
+ */
+const char* LocationInfo::getShortFileName() const{
+ if(shortFileName){
+ return shortFileName;
+ }
+
+ if(fileName == nullptr){
+ return nullptr;
+ }
+
+ const char* begin = fileName;
+ size_t pos = 0;
+ while( fileName[pos] != 0 ){
Review comment:
Opinion: Newlines before/after the loop increase readability.
##########
File path: src/main/include/log4cxx/spi/location/locationinfo.h
##########
@@ -56,6 +56,11 @@ class LOG4CXX_EXPORT LocationInfo
const char* const functionName,
int lineNumber);
+ LocationInfo( const char* const fileName,
+ size_t shortFileNameOffset,
Review comment:
This line looks a bit strange, does it follow the line above or should those underneath?
##########
File path: src/main/include/log4cxx/spi/location/locationinfo.h
##########
@@ -132,9 +152,28 @@ class LOG4CXX_EXPORT LocationInfo
#if !defined(__LOG4CXX_FUNC__)
#define __LOG4CXX_FUNC__ ""
#endif
+
+#if __cpp_lib_string_view || (_MSVC_LANG >= 201703L)
+#include <string_view>
+
+#define LOG4CXX_LOCATION_CREATE ::std::string_view file_name{__FILE__};\
Review comment:
I don't get the point of this macro: Isn't this calculated at runtime as well, only differently than `getShortFileName`? Why not have two different calculations of `getShortFileName` in the class instead of a macro containing complex implementation, making debugging more difficult in case of problems? Is there some `constexpr` or `static` missing to calculate only once?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org