Skip to content

Make headers self-contained #28942

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

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Conversation

oToToT
Copy link
Contributor

@oToToT oToToT commented Feb 12, 2025

It seems like most of the headers in OpenVINO are self-contained, but some are not. It will be nice to make every headers self-contained. This commit fixes some headers found not to be self-contained, but this is not a complete list. Just some headers found during my local compilation.

note: I can separate this commit to multiple commits if needed.

@oToToT oToToT requested review from a team as code owners February 12, 2025 04:22
@github-actions github-actions bot added the category: Core OpenVINO Core (aka ngraph) label Feb 12, 2025
@sys-openvino-ci sys-openvino-ci added the ExternalPR External contributor label Feb 12, 2025
@ilya-lavrenov ilya-lavrenov added this to the 2025.1 milestone Feb 12, 2025
@ilya-lavrenov
Copy link
Contributor

build_jenkins

@@ -4,7 +4,13 @@

#pragma once

#include <cstddef>

#ifdef OPENVINO_CPP_17_VER
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not required as CPP17 is default standard

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it mean that we also want to remove the check in L16?

Copy link
Contributor

@praasz praasz Feb 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we moved to CPP17 as default standard, this file could be even removed, and use std::optional instead ov::optional or just leave alias.

If file not remove could you update to copyright note to:

// Copyright (C) 2018-2025 Intel Corporation
// SPDX-License-Identifier: Apache-2.0
//

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed in #28949

Copy link
Contributor Author

@oToToT oToToT Feb 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll mark this conversation as resolved and rebase this PR once that PR got merged.

@oToToT oToToT requested review from a team as code owners February 13, 2025 11:16
@github-actions github-actions bot added category: inference OpenVINO Runtime library - Inference category: CPU OpenVINO CPU plugin labels Feb 13, 2025
@barnasm1 barnasm1 requested a review from praasz February 17, 2025 08:54
github-merge-queue bot pushed a commit that referenced this pull request Feb 20, 2025
### Description:
As CPP17 is the default standard of OpenVINO, there is no need to use
the self-implemented optional struct.

Note: discussed in
#28942.

### Related PRs:
- #28942

Co-authored-by: Michal Lukaszewski <[email protected]>
@mlukasze
Copy link
Contributor

#28949 has been merged
I think we can proceed with this one now?
Please, resolve conflicts and let us know can we help with something.

@oToToT oToToT marked this pull request as draft February 21, 2025 06:38
@oToToT oToToT force-pushed the ov-build-fix branch 2 times, most recently from 164b6f4 to ffec4b9 Compare February 21, 2025 09:43
@github-actions github-actions bot added the category: CPP API OpenVINO CPP API bindings label Feb 21, 2025
It seems like most of the headers in OpenVINO are self-contained, but
some are not. It will be nice to make every headers self-contained.
This commit fixes some headers found not to be self-contained, but this
is not a complete list. Just some headers found during my local
compilation.
@ilya-lavrenov ilya-lavrenov removed this from the 2025.1 milestone Apr 1, 2025
@mlukasze
Copy link
Contributor

hey @oToToT, will you find a time to finish this PR? could be kinds useful for us :)

@oToToT
Copy link
Contributor Author

oToToT commented Apr 16, 2025

Apologize that I slow down the process because it's a bit larger than I expected earlier. However, I'm still interested working on this. And I do have some progress more than this PR on my local branch.

@mlukasze
Copy link
Contributor

That's great!
ask any questions that may help you to finilize topic, reviewers will try to support you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: Core OpenVINO Core (aka ngraph) category: CPP API OpenVINO CPP API bindings category: CPU OpenVINO CPU plugin category: inference OpenVINO Runtime library - Inference ExternalPR External contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants