remove sqlite, rs2::recording_context & ::mock_context by maloel · Pull Request #11345 · realsenseai/librealsense

maloel · 2023-01-23T09:52:12Z

SQL usage is by rs2::recording_context and rs2::mock_context, both only in use in the legacy live-tests but never actually run. These are removed, as well, and rs2_create_recording_context/_mock_context are deprecated.

Tracked on [LRS-625]

remibettan · 2023-01-24T13:53:27Z


 /**
 * Create librealsense context that will try to record all operations over librealsense into a file
+ * \deprecated


Please explain what do you mean by deprecated? what will happen if someone calls this method? Will he get an exception? It would be great if this would be written besides the "deprecated" comment

It means the function will be marked as deprecated, as the Doxygen documentation states.
If you check rs.cpp, you'll see that it now throws an exception with the text deprecated...

remibettan · 2023-01-24T13:55:28Z

-        standard,
-        record,
-        playback
+        standard


do we need to let this enum, now that it has only one unique value?

I started removing it, but kept it because of changed in the dds branch.

you mean there is another value in this enum for the dds branch?

Not another value -- other constructors, one with no parameters and another one, with another parameter type.

remibettan · 2023-01-24T13:56:16Z

-            const char* section = nullptr,
-            rs2_recording_mode mode = RS2_RECORDING_MODE_COUNT,
-            std::string min_api_version = "0.0.0");
+        explicit context( backend_type type );


If you choose to keep this enum, maybe we should add as default value the unique value. Agree?

No. On the 'dds` branch there's a default ctor, so this distinguishes it from another usage.

So maybe there is an issue there, because it does not seem right that "standard" is not default, even if there are other possible values.
Let's talk about it tomorrow, please

I want it to be explicit, just like it has been until now. You want this constructor, you specify standard.

remibettan · 2023-01-24T13:56:52Z

 #include "sr300-fw-update-device.h"
 #include "sr300.h"
 #include "ivcam-private.h"
+#include <librealsense2/h/rs_internal.h>


Please confirm this is really needed

Yes, needed -- I removed an #include in context.h which included this one. Not that it's removed, I needed to add it elsewhere.

remibettan · 2023-01-24T13:57:50Z

 #include "firmware_logger_device.h"
 #include "device-calibration.h"
 #include "calibrated-sensor.h"
+#include <librealsense2/h/rs_internal.h>


Please confirm this is needed here

remibettan · 2023-01-24T13:59:40Z

    verify_version_compatibility(api_version);

-    return new rs2_context{ std::make_shared<librealsense::context>(librealsense::backend_type::standard) };
+    return new rs2_context{ std::make_shared< librealsense::context >( librealsense::backend_type::standard ) };


I do not like these spaces beside the "<>", but... your style :-)

No, it's our style.
There is a .clang-format file that you should be using. See me tomorrow if you do not :)

ok - feel free to send it to me

remibettan · 2023-01-24T14:01:07Z

-    verify_version_compatibility(api_version);
-
-    return new rs2_context{ std::make_shared<librealsense::context>(librealsense::backend_type::record, filename, section, mode) };
+    throw not_implemented_exception( "deprecated" );


This answers one of my previous comments, but please add there the fact that exception is thrown when calling the deprecated api methods.

No. A deprecated method should not be called. If someone does, he get an exception.

remibettan · 2023-01-24T14:02:33Z

 inline bool make_context(const char* id, rs2::context* ctx, std::string min_api_version = "0.0.0")
 {
    rs2::log_to_file(RS2_LOG_SEVERITY_DEBUG);
+    min_api_version;


Please confirm this line is needed, and explain why if it is.

To avoid "unused variable" warnings.

Sorry, but I do not agree - I prefer to get a warning than seeing this - don't you?

Normally the usual way to do it is (that I have seen):
(void) min_api_version;

You are correct, but I still do not understand exactly why... there is also std::ignore. I'll fix it in the dds branch when I merge there?

remibettan · 2023-01-24T14:03:22Z

-    }
-
+    command_line_params::instance()._found_any_section = true;
+    return true;


Should this method be kept if this is the only things it does?

It's also modified, but differently, on the dds branch. Live tests are old and we'll not invest much in improving them. For now this change you're suggesting may happen, but in a different PR.

ok, please open a ticket for that

No, Remi. We won't waste time on live-tests. I'll do it if I happen to be messing around in live-tests again. Leave it be... you're welcome to open a ticket if you insist.

One thing I can offer is that right after we merge this I'll be merging with the dds branch, where there will be conflicts. I'll take a look then to see what can be removed.

remibettan · 2023-01-24T14:04:27Z

 #include "core/streaming.h"
 #include "device.h"
 #include "context.h"
+#include <librealsense2/h/rs_internal.h>


Please confirm this line is needed

remibettan

Few comments added - thanks!

maloel · 2023-01-24T15:14:30Z

Few comments added - thanks!

Thank You :)

remibettan

LGTM

maloel requested a review from Nir-Az January 23, 2023 09:52

Nir-Az requested a review from remibettan January 23, 2023 18:40

remove sqlite, rs2::recording_context & ::mock_context

863fb47

maloel force-pushed the nosql branch from f24ae58 to 863fb47 Compare January 24, 2023 13:22

update cppcheck_run.log

38bf7be

remibettan reviewed Jan 24, 2023

View reviewed changes

remibettan self-requested a review January 25, 2023 08:01

remibettan approved these changes Jan 25, 2023

View reviewed changes

maloel removed the request for review from Nir-Az January 25, 2023 08:30

maloel merged commit c137903 into realsenseai:development Jan 25, 2023

maloel deleted the nosql branch January 25, 2023 08:31

Uh oh!

Conversation

maloel commented Jan 23, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

remibettan Jan 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

remibettan Jan 24, 2023 •

edited

Loading