PlatformLab
std::string std::string_view support
How much work would it be to add support for std::string and std::string_view and thus avoid the use of strlen() in the hotpath?
Currently using std::string_view is quite problematic as there's no guarantee that it's null terminated so you have to construct a std::string and then call c_str() on it before passing it into the logger. Considering the length is already encoded into the std::strig_view this is very wasteful.
Hm... I think the answer depends on which version of NanoLog you intend to modify. It should be fairly simple to do in C++17, and a bit harder in Preprocessor NanoLog.
I'll start with the C++17 answer. I suspect you'll want to be able to pass in the string_view object to the log function and have the interface resemble something like ``NANO_LOG(WARNING, "%s", stringView)''. In this case, you'll need to modify/add 2-3 functions to make this work.
-
First, NanoLog needs to understand how to get the length of the string. To do this you'll need to add another overloaded getArgSize() function to accept std::string_view objects. This is the only function that will ever get the length of the string. Afterwards, the information is stored internally.
-
Next, NanoLog needs to know how to copy the string_view data into its internal buffers. To do this, you'll have to add another template specialization for the store_argument() function to specialize on std::string_view. Conceptually, this function reads the string data from "T arg" and stores a length field plus the raw character data into "char** storage." I think the simplest way to do this one is probably to add a new store_argument function that specializes on string_view's and then immediately invokes store_argument<const char*>(storage, arg.data, paramType, stringSize). In other words, it's just a wrapper that peels the string_view off and passes the char* pointer down to the same logic. You'll probably need to add more of the std::enable_if<std::is_same<...>> conditions to make sure the compiler properly separates other specializations from std::string_view's.
-
At this point, everything will probably work. But if not, you may also need to add a specialization for the compressSingle function. This function copies the string from an input buffer to an output buffer. However, I suspect the included function will just work.
I'll skip the preprocessor version of NanoLog for now. Let me know if you need additional help!
Thanks, that makes sense. I had a look at the code and I think the sticking point may be the printf attribute checking. An I correct in saying all format string/args validation is handle by the compiler using the printf attribute? If so does that mean a complete new implementation would have to be done in order to add string_view?! That would be rather a lot of work, especially given the lack of a starting point.
On Thu, 13 Feb 2020, 6:31 am syang0, [email protected] wrote:
Hm... I think the answer depends on which version of NanoLog you intend to modify. It should be fairly simple to do in C++17, and a bit harder in Preprocessor NanoLog.
I'll start with the C++17 answer. I suspect you'll want to be able to pass in the string_view object to the log function and have the interface resemble something like ``NANO_LOG(WARNING, "%s", stringView)''. In this case, you'll need to modify/add 2-3 functions to make this work.
First, NanoLog needs to understand how to get the length of the string. To do this you'll need to add another overloaded getArgSize() https://github.com/PlatformLab/NanoLog/blob/955fc375dc2a152584e4468f3284885fd12218d1/runtime/NanoLogCpp17.h#L600 function to accept std::string_view objects. This is the only function that will ever get the length of the string. Afterwards, the information is stored internally. 2.
Next, NanoLog needs to know how to copy the string_view data into its internal buffers. To do this, you'll have to add another template specialization for the store_argument() https://github.com/PlatformLab/NanoLog/blob/955fc375dc2a152584e4468f3284885fd12218d1/runtime/NanoLogCpp17.h#L397 function to specialize on std::string_view. Conceptually, this function reads the string data from "T arg" and stores a length field plus the raw character data into "char** storage." I think the simplest way to do this one is probably to add a new store_argument function that specializes on string_view's and then immediately invokes store_argument<const char*>(storage, arg.data, paramType, stringSize). In other words, it's just a wrapper that peels the string_view off and passes the char* pointer down to the same logic. You'll probably need to add more of the std::enable_if<std::is_same<...>> conditions to make sure the compiler properly separates other specializations from std::string_view's. 3.
At this point, everything will probably work. But if not, you may also need to add a specialization for the compressSingle https://github.com/PlatformLab/NanoLog/blob/955fc375dc2a152584e4468f3284885fd12218d1/runtime/NanoLogCpp17.h#L742 function. This function copies the string from an input buffer to an output buffer. However, I suspect the included function will just work.
I'll skip the preprocessor version of NanoLog for now. Let me know if you need additional help!
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/PlatformLab/NanoLog/issues/23?email_source=notifications&email_token=AL5LS3CFGAZB73UFTACKB4TRCSWH7A5CNFSM4KTY3PEKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELTE7QI#issuecomment-585519041, or unsubscribe https://github.com/notifications/unsubscribe-auth/AL5LS3AYRHJBHDDEERXKXDLRCSWH7ANCNFSM4KTY3PEA .
Ah! That is true, C++17 NanoLog relies on the GNU format attribute to check for errors. That is a complication I did not foresee.
Hm... I think it is doable though. I already have a function that parses the format strings at compile-time to extract type information called getParamInfo(). I could imagine one using this function as a starting point and modifying where it checks for terminals (Line 203) to also do a type check.