Add config-read and run thread_birds with arguments.

[Markuu-s]

No description provided.

[ol-imorozko]

Suggested change

	inline static const std::string socketStr = "socket";
	inline static const std::string vrfStr = "vrf";
	static constexpr auto socketStr = "socket";
	static constexpr auto vrfStr = "vrf";

[Markuu-s]

1. "constexpr" doesn`t include "inline" in all compilers
2. I thought about "constexpr" instead of "const" buf std::string doesn`t support it
3. auto give us const char[N], I think in C++ it worst

[ol-imorozko]

1. Yeah, I think we do need inline here because we need that field to be ODR-usable: link

A static data member may be declared inline. An inline static data member can be defined in the class definition and may specify an initializer. It does not need an out-of-class definition:

struct X
{
    inline static int fully_usable = 1; // No out-of-class definition required, ODR-usable
    inline static const std::string class_name{"X"}; // Likewise
 
    static const int non_addressable = 1; // C.f. non-inline constants, usable
                                          // for its value, but not ODR-usable
    // static const std::string class_name{"X"}; // Non-integral declaration of this
                                                 // form is disallowed entirely
};

We're binding it to a reference by passing it to exist function => ODR-usage => we must provide exactly one definition of that static data member in the entire program and the simplest way to do so -- use inline here.

2. Yeah, that's correct. Here you can use std::string_view for string literals, but the issue with it is that nlohmann::json doesn't have an overload for operator[] with std::string_view. However, it does have overloads for std::string and const char*. So you could either use the change I suggested, or go with std::string_view and pass .data() like this:

if (exist(elemJson, BirdImport::socketStr))
{
    import.socket = elemJson[BirdImport::socketStr.data()];
}

if (exist(elemJson, BirdImport::vrfStr))
{
    import.vrf = elemJson[BirdImport::vrfStr.data()];
}

Either way works for me.

3. Why do you think so? IMHO using array types isn't inherently bad in C++. It's quite common and valid (as long as we don't decay them into pointers and/or perform pointer arithmetic :) ). I don't see any issues with using array types in this context.

[ol-imorozko]

Is every module is also a rib module? I don't quite understand.
Also, dynamic_cast will inevitably lead to some errors, if, for example, we decide to change class hierarchy.
Can we use other things here? Like maybe we can utilize one or two virtual functions?

[Markuu-s]

At this stage, I need to call one method that is only in rib_t (bird_import_get()), using a virtual function for all classes is somehow strange. I'll think about it, or I'm ready to discuss what's best.

[ol-imorozko]

modules is a vector of cModule pointers. So what you can do is add a new field to the class that will point to the required module, and here just call it's methods.
Something like an iterator to the modules will suffice (but be aware of invalidation). Or just a pointer/reference to rib_t.
Either way it's better than what we have how.

[ol-imorozko]

We try to avoid "merge commits" in the history. You can simply squash that commit with "Add config-read and run thread_birds with arguments." In fact, commits like "Fix PR" are not desirable either. Commits should be self-contained, i.e., if you are improving/changing something after a review, it's better to introduce those changes within the commit itself. Here, you can just squash your four commits into one: "Add config-read and run thread_birds with arguments."

Also, could you please add a small description to commit body? It's hard to grasp what "run thread_birds with arguments" mean from commit title.

[ol-imorozko]

What if those keys do not exist? We need to use some default value then, right? Otherwise I think it will be two empty strings.
It's better to use value(const typename object_t::key_type& key, const ValueType& default_value) method of nlohmann::json like so:

Suggested change

	if (exist(elemJson, BirdImport::socketStr))
	{
	import.socket = elemJson[BirdImport::socketStr];
	}
	if (exist(elemJson, BirdImport::vrfStr))
	{
	import.vrf = elemJson[BirdImport::vrfStr];
	}
	import.socket = elemJson.value(BirdImport::socketStr, "some_default_value");
	import.vrf = elemJson.value(BirdImport::vrfStr, "some_default_value");