Skip to content

Conversation

@chipbarnaby
Copy link
Contributor

@chipbarnaby chipbarnaby commented Jan 19, 2024

Description

Motivation: capture and report btwxt error messages.

Implemented several classes derived from Courier. Updated all btwxt uses.

No results changes. No documentation changes.

@chipbarnaby chipbarnaby marked this pull request as draft January 19, 2024 21:32
@nealkruis
Copy link
Contributor

Here's how I would design this:

In hvac.h:

class RSYSBtwxtLogger : public Courierr::Courierr
{
public:
	explicit RSYSBtwxtLogger(std::string rgi_name_in, RSYS* pRSYS_in) : rgi_name(rgi_name_in), pRSYS(pRSYS_in) {}
	void error(const std::string_view message) override { pRSYS->oer(format_message(message)); }
	void warning(const std::string_view message) override { pRSYS->oWarn(format_message(message)); }
	void info(const std::string_view message) override { pRSYS->oInfo(format_message(message)); }
	void debug(const std::string_view message) override {  pRSYS->oInfo(format_message(message));  }
private:
	RSYS* pRSYS;
	std::string rgi_name;
	std::string format_message(std::string message) { return fmt::format("btwxt '{}' -- {}", rgi_name, message)}
}

and I'd make a similar one for CHDW. You could even put rgi_name and format_message into a baseclass that these are both derived from. That way you don't have to deal with the callbacks and casting.

@chipbarnaby chipbarnaby marked this pull request as ready for review February 16, 2024 19:51
@chipbarnaby chipbarnaby assigned nealkruis and unassigned nealkruis Feb 16, 2024
Copy link
Contributor

@nealkruis nealkruis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider changes. I won't push too hard on my suggestions if you want to merge anyway.

Comment on lines +39 to +63
class CourierMsgHandler : public CourierMsgHandlerBase
{
public:
using MsgCallbackFunc = void(void* pContext, MSGTY msgty, const char* msg);

CourierMsgHandler(MsgCallbackFunc* pMsgCallbackFunc,void* context)
: cmh_pMsgCallbackFunc(pMsgCallbackFunc), cmh_context( context)
{ }

private:
virtual void forward_message(MSGTY msgty, const std::string& crMsg) override
{
const char* msg = crMsg.c_str();
if (cmh_pMsgCallbackFunc)
(*cmh_pMsgCallbackFunc)(cmh_context, msgty, msg);
else
err(PABT, "nullptr cmh_pMsgCallbackFunc '%s'", msg);

}

private:
void* cmh_context; // caller context
MsgCallbackFunc* cmh_pMsgCallbackFunc; // pointer to callback function

}; // class CourierMsgHandler
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thought is that void* context and a MsgCallbackFunc* are very "C". There is a lot of indirection for people trying to read the code. My preference would be to make a new class for each different context with a specific forward_message override.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Don't want 10 essentially identical implementations. Maybe there is a way to template this to make it clearer? In any case, there is going to be a "callable" somewhere aka a pointer.
  2. Additionally, what is wrong with C? CSE is small and fast, partially due to using lots of C stuff.
  3. I recently learned about a "dependency injection" scheme which might be very useful here.

Comment on lines +395 to +402
char* strt_string_view( // copy string_view to temporary
std::string_view sv) // string view
{
// strncpy0 handles sv.size() = 0
char* sRet = strncpy0( nullptr, sv.data(), sv.size()+1);
return sRet;
} // :: strt_string_view
// ====================================================================
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not right now, but I made it and tested it. It is tiny and likely useful soon given the ever-growing interaction with other libraries. So I say leave it.

char * FC strtmp( const char *s);
char * CDEC strtcat( const char *s, ... );
char * CDEC strntcat( int n, ...);
char* strt_string_view(std::string_view sv);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see previous question.

@chipbarnaby chipbarnaby merged commit 650637e into main Feb 21, 2024
@chipbarnaby chipbarnaby deleted the btwxt-error-handling branch February 21, 2024 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants