245 switch to cavl2 #250
245 switch to cavl2 #250
Conversation
songmeo
force-pushed
the
245-switch-to-cavl2
branch
from
65af23f to
8625b28
Compare
songmeo
force-pushed
the
245-switch-to-cavl2
branch
from
aa59dda to
098a550
Compare
songmeo
force-pushed
the
245-switch-to-cavl2
branch
from
4e2a001 to
ac4f382
Compare
songmeo
force-pushed
the
245-switch-to-cavl2
branch
from
0a28819 to
41ec76d
Compare
songmeo
force-pushed
the
245-switch-to-cavl2
branch
from
95b8d92 to
26f8f86
Compare
songmeo
force-pushed
the
245-switch-to-cavl2
branch
3 times, most recently
from
9e82af1 to
ecfd769
Compare
songmeo
force-pushed
the
245-switch-to-cavl2
branch
from
ecfd769 to
482c9b0
Compare
pavel-kirienko
left a comment
pavel-kirienko
left a comment
There was a problem hiding this comment.
What #245 was trying to say is that cavl2 should no longer be vendored as part of libcanard itself, but rather it has to be provided by the user. One practical implication is that we need to remove it from the libcanard directory, and place it instead somewhere under lib/cavl2/cavl2.h (should it be a git submodule? probably not), which will then be added as a system include directory. Using a blanket -system for the entire libcanard directory is harmful because it will squelch bona fide warnings from the compiler concerning libcanard's own header.
Direct compiler invocations in the CI script will need to be updated accordingly.
|
Also, see if you can use |
pavel-kirienko
left a comment
pavel-kirienko
left a comment
There was a problem hiding this comment.
This Sonar report is legit, we could address it in this changeset as well since it's minor:
The other one requires a // NOSONAR comment.
songmeo
force-pushed
the
245-switch-to-cavl2
branch
2 times, most recently
from
1234c96 to
801b2d8
Compare
songmeo
force-pushed
the
245-switch-to-cavl2
branch
from
801b2d8 to
e7154c7
Compare
pavel-kirienko
left a comment
pavel-kirienko
left a comment
There was a problem hiding this comment.
Please address the remaining minor issues and then merge.
tests/CMakeLists.txt
Outdated
| include_directories(${library_dir}) | ||
| include_directories(SYSTEM catch) | ||
| include_directories(SYSTEM ${CMAKE_SOURCE_DIR}/../lib/cavl2) | ||
| include_directories(${CMAKE_SOURCE_DIR}/../libcanard) |
There was a problem hiding this comment.
Redundant; we already have include_directories(${library_dir}) on line 71
|
closes #245