Conversation
|
I confirm this is required on your Nvidia jetson platforms. |
RTProtocol.cpp
Outdated
| else | ||
| { | ||
| sprintf(maErrorStr, "No packet received. %s.", maErrorStr); | ||
| snprintf(maErrorStr, 1060, "No packet received. %s.", maErrorStr); |
There was a problem hiding this comment.
should this be part of the PR?
There was a problem hiding this comment.
Using snprintf(maErrorStr, 1060, "No packet received. %s.", maErrorStr); to avoid the compiler warning and potentially preventing buffer overflows seems reasonable. 👍 Although I suppose it could be a separate PR.
Maybe it's obvious, but where did a size of 1060 come from?
There was a problem hiding this comment.
I agree this should be s separate PR. Also curious where 1060 comes from, as maErrorStr is 1024 characters.
There was a problem hiding this comment.
I think setting it to 1024 would be fine.
|
Like @MaximilienNaveau pointed out, his approach in #45 also addresses this issue by hardcoding If there are any additional insights I should be aware of, please let me know. (: |
I believe this PR is better suited to be merge than mine I closed mine in response |
nim65s
left a comment
nim65s
left a comment
There was a problem hiding this comment.
I did the same in nim65s@c39f643, but forgot to open a PR, so thanks for this !
|
@jgoppert : could you consider adding nim65s@1ba8cdb to this PR ? |
bahlborn
left a comment
bahlborn
left a comment
There was a problem hiding this comment.
Move sprintf->snprintf change to another PR.
Verify use of 1060 as buffer size.
|
Hey! I just wanted to mention that we've taken these changes from this fork of yours and create THIS PR. Feel free to have a look at it. Note that the "sprintf" issues have already been fixed. Once the PR is approved and merged then we can close this one. Thank you very much for the input. :) |
jgoppert
force-pushed
the
install_fix
branch
2 times, most recently
from
db5e226 to
02a6035
Compare
GNUInstallDirs was not defined when target_include_directories was set. This caused it to be blank and the cmake target import failed. User projects importing the cmake target should use include qualisys_cpp_sdk/RTProtocol.h etc. Signed-off-by: James Goppert <james.goppert@gmail.com>
jgoppert
force-pushed
the
install_fix
branch
from
02a6035 to
c1f19c4
Compare
|
I simplified this PR and just make the GNUInstallDirs fix. |
|
@OliverGlandberger @nim65s maybe we can make the other changes another PR and close this one for simplicity? |
|
If it makes more sense to take it all together with #65 I'm fine with that. Just happy to see it merged :-) |
6 participants
GNUInstallDirs was not defined when target_include_directories was set. This caused it to be blank and the cmake target import failed. User projects importing the cmake target should use include qualisys_cpp_sdk/RTProtocol.h etc.