-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
clean up types.h #12372
clean up types.h #12372
Conversation
#include "reflectivity.h" | ||
#include <types.h> | ||
#include <vector> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, Reflectivity & MUR can both be deleted totally., not used.
Both are L515 only and was origin by L515 reflectivity issues
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I know, we spoke about it and, at least for now, I decided to leave them alone. Let's discuss offline.
@@ -126,6 +127,8 @@ namespace librealsense | |||
<< intrinsics_string(res_1280_800) | |||
<< intrinsics_string(res_960_540)); | |||
|
|||
#undef intrinsics_string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed? even on cpp file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, just a good practice
namespace librealsense { | ||
|
||
|
||
using firmware_version = rsutils::version; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be a problem.
Better to specifically use rsutils::version,
firmware_version is spread around the code as local var and we always do using librealsense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just moved it in this PR. If you want I can work to change but it's a whole lot of changes in many many places.
|
||
frame_callback_ptr rv = { | ||
new internal_frame_callback<decltype(to_syncer)>(to_syncer), | ||
[](rs2_frame_callback* p) { p->release(); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for this release anymore?
Just need to make sure no memory leak here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's still there: make_frame_callback
adds it automatically.
Still some stuff left, but I figured this was enough for now (it's been building up).
Some are bigger than others. I recommend comparing commits one at a time to make this more understandable.
No functional changes.
Tracked on [LRS-923]