-
Notifications
You must be signed in to change notification settings - Fork 407
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
iox-1391 Move vocabulary types
to separate module
#1791
iox-1391 Move vocabulary types
to separate module
#1791
Conversation
aa126fe
to
ef4a63e
Compare
Codecov Report
@@ Coverage Diff @@
## master #1791 +/- ##
==========================================
- Coverage 75.63% 75.61% -0.02%
==========================================
Files 375 375
Lines 14578 14569 -9
Branches 2064 2064
==========================================
- Hits 11026 11017 -9
+ Misses 2922 2921 -1
- Partials 630 631 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
097456a
to
b6b29f0
Compare
b6b29f0
to
2f01740
Compare
a19aebc
to
1276091
Compare
iceoryx_hoofs/vocabulary/include/iox/detail/string_type_traits.hpp
Outdated
Show resolved
Hide resolved
300b9a9
to
48f45f1
Compare
eb6bf6f
to
528df20
Compare
2ab0936
to
431a94e
Compare
Signed-off-by: Simon Hoinkis <simon.hoinkis@apex.ai>
…string' to vocabulary module Signed-off-by: Simon Hoinkis <simon.hoinkis@apex.ai>
…ected', 'variant' and 'string'' Signed-off-by: Simon Hoinkis <simon.hoinkis@apex.ai>
…hanges Signed-off-by: Simon Hoinkis <simon.hoinkis@apex.ai>
…ectory Signed-off-by: Simon Hoinkis <simon.hoinkis@apex.ai>
… fix some typos Signed-off-by: Simon Hoinkis <simon.hoinkis@apex.ai>
…tform' Signed-off-by: Simon Hoinkis <simon.hoinkis@apex.ai>
Please fix the links to the |
Signed-off-by: Simon Hoinkis <simon.hoinkis@apex.ai>
Good catch, done ✔️ |
d972b3d
to
e3cff34
Compare
…ary namespace and add implicit includes Signed-off-by: Simon Hoinkis <simon.hoinkis@apex.ai>
e3cff34
to
eb861cc
Compare
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.
Looks good but please update ./clang-tidy-diff-scans.txt
.
…tidy scan Signed-off-by: Simon Hoinkis <simon.hoinkis@apex.ai>
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.
Half way through :)
Please do a git checkout master iceoryx_posh
and make it compile. This ensures that all symbols are available via the legacy includes.
In general I think having only one base namespace makes the code more readable.
iceoryx_hoofs/vocabulary/include/iox/detail/string_type_traits.hpp
Outdated
Show resolved
Hide resolved
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.
Second half done :)
@elBoberido Thanks for the hint!
This revealed all the places in |
17c8140
to
acf7aa9
Compare
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.
Looks good. Almost there
namespace iox | ||
{ | ||
/// @todo iox-#1593 Deprecate include | ||
/// [[deprecated("Deprecated in 3.0, removed in 4.0, please include 'iox/expected.hpp' instead")]] |
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'm wondering a bit when this deprecation warning would be shown. When the cxx
namespace will be used? In this case the deprecation message would be wrong
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.
@elBoberido I tried this and had a look very early on when we started with #1391. This will only deprecated what is in exactly this very namespace definition and not everything in cxx
:D
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.
Okay, tried it out. Deprecating namespaces is a C++17 feature and would indeed print it everywhere where the namespace is used. I think we have to add this comment to the respective using directive :(
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.
@elBoberido Odd, I tried that 🤔 According to cppref it's C++14: https://en.cppreference.com/w/cpp/language/attributes/deprecated
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.
Ah, I might know why it didn't work for you. I just put the comment above because it's so long.. IIRC you have to write:
namespace [[deprecated]] foo
{
// bla..
}
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 get this warning from my IDE Attributes on a namespace declaration are a C++17 extension [-Wc++17-extensions]
and when compiling with clang but not with gcc
Thinking about it, we could use this trick to ensure that we do not break the API. A CI job which builds posh 3.0 against hoofs master and examples 3.0 against posh + hoofs master. |
…ing consistently in the docs, moving TypeInfo back and importing other related API calls Signed-off-by: Simon Hoinkis <simon.hoinkis@apex.ai>
acf7aa9
to
9a8f14d
Compare
IMHO that would be overkill... |
well, it depends on how serious we are about API breakage 😅 we might not yet be there but at some point I think it would make sense |
Pre-Review Checklist for the PR Author
iox-123-this-is-a-branch
)iox-#123 commit text
)task-list-completed
)iceoryx_hoofs
are added to./clang-tidy-diff-scans.txt
Notes for Reviewer
optional
,expected
,variant
andstring
to their own modulevocabulary types
cxx
namespace for those four classesis_cxx_string
type trait out of the generaltype_traits.hpp
into its own fileChecklist for the PR Reviewer
iceoryx_hoofs
have been added to./clang-tidy-diff-scans.txt
Post-review Checklist for the PR Author
References
iceoryx_hoofs
into logical modules #1391 (partly)