-
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
Remove static from API (rsutil.h) #9096
Remove static from API (rsutil.h) #9096
Conversation
1072e43
to
1d73b87
Compare
1d73b87
to
6f49d65
Compare
remove redundant file
added empty line
6f49d65
to
ce179ad
Compare
} | ||
} | ||
} | ||
NOEXCEPT_RETURN(, to_pixel, data) |
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.
We don't want the other params? (same for other functions... missing args)
set(CMAKE_EXPORT_COMPILE_COMMANDS 1) | ||
|
||
add_executable(unit-tests-rsutil-c C/unit-tests-rsutil-c.c ../../include/librealsense2/rsutil.h) | ||
include_directories(../C) |
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.
What's this for?
# minimum required cmake version: 3.1.0 | ||
cmake_minimum_required(VERSION 3.1.0) | ||
|
||
project(unit-tests-rsutil-c) |
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 would have expected:
project(unit-tests-rsutil-c C)
To make is a C compile...
How is this a C compilation otherwise?
project(unit-tests-rsutil-c) | ||
|
||
# Save the command line compile commands in the build output | ||
set(CMAKE_EXPORT_COMPILE_COMMANDS 1) |
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.
Was this for debug? Still needed?
|
||
printf("SUCCEED"); | ||
|
||
return 1; |
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.
1
is a failure...
#include <stdio.h> | ||
|
||
#ifdef __cplusplus | ||
extern "C" { |
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.
Since this is supposed to be compiled in C only, I'd expect to see #error
here.
@@ -0,0 +1,31 @@ | |||
// License: Apache 2.0. See LICENSE file in root directory. |
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 (unit-tests/unit-tests-rsutil.cpp
) is a repetition of the C code.
I honestly think a better solution would have been a Python script that calls these functions. No CMakes, no compilation, nothing weird, and it could have been included in LibCI.
{ | ||
rs2::context ctx; | ||
|
||
if (make_context(SECTION_FROM_TEST_NAME, &ctx)) |
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.
Why do you need a context?
This test isn't live...
Superseded in #9379 |
Tracked on: DSO-17046
Still need to be done:
move internal functions implementation from rs.cpp (namely: next_pixel_in_line, is_pixel_in_line, adjust_2D_point_to_boundary)-Done, moved implementation to rsutil.h as inlinecreate a sub-directory for C unit-test (figure out where, maybe change names)-Done, added both to live-testscheck on Linux (maybe Travis is enough?)Done, passed Travis