[API] Fix inclusion header files and use forward declaration #2124
Conversation
cngzhnp
force-pushed
the
feature/api/forward_declaration
branch
14 times, most recently
from
5e58615 to
0baef79
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #2124 +/- ##
==========================================
- Coverage 87.19% 87.15% -0.03%
==========================================
Files 166 166
Lines 4784 4777 -7
==========================================
- Hits 4171 4163 -8
- Misses 613 614 +1
|
cngzhnp
force-pushed
the
feature/api/forward_declaration
branch
2 times, most recently
from
479bdcc to
2690363
Compare
cngzhnp
force-pushed
the
feature/api/forward_declaration
branch
from
2690363 to
2edcbe4
Compare
|
@marcalff Could you please review these changes only related to API folder? |
|
Changes look good in general, and nicely done. I think this would be the breaking change for applications if they are taking dependency on these unnecessary header inclusions in our API. These applications need to explicitly include these removed header files / forward-declared classes. |
There was a problem hiding this comment.
Approving the changes with nit comments. I believe @marcalff has already approved these api changes as indicated here - #2043 (review). Still have concern on this breaking the existing application code, so would be good to discuss when/how to merge/release in next community meeting.
| @@ -41,6 +40,11 @@ | |||
| OPENTELEMETRY_BEGIN_NAMESPACE | |||
| namespace plugin | |||
| { | |||
|
|
|||
| struct LoaderInfo; | |||
| class Factory; | |||
There was a problem hiding this comment.
No need to put this class as a forward declaration, probably even stands on my early trials. Will be removed.
| @@ -102,15 +102,14 @@ class OPENTELEMETRY_EXPORT NoopTracer final : public Tracer, | |||
| /** | |||
| * No-op implementation of a TracerProvider. | |||
| */ | |||
| class OPENTELEMETRY_EXPORT NoopTracerProvider final : public opentelemetry::trace::TracerProvider | |||
| class OPENTELEMETRY_EXPORT NoopTracerProvider final : public trace_api::TracerProvider | |||
There was a problem hiding this comment.
should we instead use trace::TracerProvider, and remove trace_api namespace alias from the header file. It's not recommended to have namespace alias in header files (though this is existing code)
There was a problem hiding this comment.
Yes, you are right. I'd change it as you wish.
There was a problem hiding this comment.
Looks like the trace_api alias defined here is been used in other places too, as the CI tests are failing. We can keep it here in that case. Sorry about that.
There was a problem hiding this comment.
Reverted back only trace_api alias. However, I will take it as a note, maybe it would be removed after same changes are applied to SDK too.
cngzhnp
force-pushed
the
feature/api/forward_declaration
branch
from
296472a to
2f78c15
Compare
api/test/plugin/dynamic_load_test.cc
Outdated
| @@ -2,6 +2,12 @@ | |||
| // SPDX-License-Identifier: Apache-2.0 | |||
|
|
|||
| #include "opentelemetry/plugin/dynamic_load.h" | |||
| #ifdef _WIN32 | |||
There was a problem hiding this comment.
Why is this needed as the #ifdef _WIN32 is included in opentelemetry/plugin/dynamic_load.h which has already been included above?
There was a problem hiding this comment.
In dynamic_load.h file, there is no preprocessor definition anymore, I removed it in this pull request. Because there is only one function declaration which is not related to operating system itself.
However, only this source file is related to operating system itself with dynamic_load and which has to be included like that. Otherwise, there will be a compiler error on test codes.
There was a problem hiding this comment.
Why was the including of such dynamic_load_<os>.h moved out of dynamic_load.h? With change, the user code like the test code here need to include both files separately. It looks to me that including the os header in dynamic_load.h is more convenient.
There was a problem hiding this comment.
Ok, I will revert it back.
marcalff
changed the title
marcalff
changed the title
Fixes #2043 but only API side.
Changes
Please provide a brief description of the changes here.
For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes