Skip to content

style: use full paths for internal headers in capi/...#313

Open
VDanielEdwards wants to merge 2 commits into
mainfrom
dev/refactor/use-full-include-paths-capi
Open

style: use full paths for internal headers in capi/...#313
VDanielEdwards wants to merge 2 commits into
mainfrom
dev/refactor/use-full-include-paths-capi

Conversation

@VDanielEdwards
Copy link
Copy Markdown
Member

@VDanielEdwards VDanielEdwards commented Apr 24, 2026

Subject

Use full paths for internal headers in all sources and headers in SilKit/source/capi/. Rearranges the includes to follow the guidelines in the LLVM Styleguide for C++ Google Styleguide for C++. Removes unneccessary includes.

Developer checklist (address before review)

  • Changelog.md updated
  • Prepared update for depending repositories
  • Documentation updated (public API changes only)
  • API docstrings updated (public API changes only)
  • Rebase → commit history clean
  • Squash and merge → proper PR title

@VDanielEdwards VDanielEdwards self-assigned this Apr 24, 2026
@snps-behrens
Copy link
Copy Markdown
Collaborator

Is there any particular reason you chose the google style guide? I have found that using the llvm style guide leads to headers being more self-contained.

Signed-off-by: Daniel Edwards <Daniel.Edwards@vector.com>
@VDanielEdwards VDanielEdwards force-pushed the dev/refactor/use-full-include-paths-capi branch from d46ede3 to dda8abe Compare April 24, 2026 13:13
@VDanielEdwards
Copy link
Copy Markdown
Member Author

Is there any particular reason you chose the google style guide? I have found that using the llvm style guide leads to headers being more self-contained.

No particular reason, it was just the first that came to mind. But I do like the ordering from local -> global as in the LLVM styleguide more than what the Google styleguide suggests.

@MariusBgm would you object to switching the include-order to the LLVM styleguide?

Signed-off-by: Daniel Edwards <Daniel.Edwards@vector.com>
@VDanielEdwards VDanielEdwards requested review from snps-behrens and removed request for snps-behrens April 27, 2026 11:19
@VDanielEdwards VDanielEdwards added the needs reviewer This issue is looking for a reviewer. label Apr 29, 2026
@snps-fiodorov snps-fiodorov requested a review from snps-behrens May 5, 2026 09:22
@snps-fiodorov snps-fiodorov removed the needs reviewer This issue is looking for a reviewer. label May 5, 2026
Copy link
Copy Markdown
Collaborator

@snps-behrens snps-behrens left a comment

Choose a reason for hiding this comment

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

Looks pretty good. I did notice you removed pretty much all system includes from these files, even where there is extensive use of the standard library. Is that intentional?

#include "silkit/participant/IParticipant.hpp"
#include "silkit/services/orchestration/all.hpp"

#include <map>
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why remove mutex here? It's still used in this file.

#include <mutex>
#include <fstream>
#include "silkit/participant/IParticipant.hpp"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why remove memory here?

#include "capi/CapiImpl.hpp"
#include "config/ParticipantConfiguration.hpp"

#include "silkit/vendor/ISilKitRegistry.hpp"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

same here (memory)?

@@ -6,12 +6,15 @@
#define _CRT_SECURE_NO_WARNINGS
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Not really in scope for this PR, but would it make sense to set this flag globally, rather than in every file where these warnings trigger (or have triggered in the past)?

e.g. by adding something like $<$<COMPILE_LANG_AND_ID:CXX,MSVC>:_CRT_SECURE_NO_WARNINGS> to the global add_compile_definitions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants