Skip to content

Conversation

@atta-ullah01
Copy link

@atta-ullah01 atta-ullah01 commented Jan 29, 2026

Replaces C-string path handling with std::filesystem::path.

Key Changes

Path Handling:

  • Converted p4includePath and p4_14includePath to std::filesystem::path
  • Added Windows compatibility in isSystemFile by enforcing generic path separators (/)
  • Removed strdup usage in tests

@atta-ullah01 atta-ullah01 force-pushed the fix/cpp20-modernization branch 2 times, most recently from 3b32f69 to 7a6bb25 Compare January 30, 2026 09:52
@atta-ullah01 atta-ullah01 force-pushed the fix/cpp20-modernization branch from 7a6bb25 to 23ed3f7 Compare January 30, 2026 10:42
@atta-ullah01 atta-ullah01 changed the title Modernize: Use std::filesystem::path and C++20 Concepts Modernize: Use std::filesystem::path for p4includePath Jan 30, 2026
if (includesEmitted.find(sourceFile) == includesEmitted.end()) {
if (sourceFile.startsWith(p4includePath)) {
const char *p = sourceFile.c_str() + strlen(p4includePath);
if (sourceFile.startsWith(p4includePath.generic_string().c_str())) {
Copy link
Collaborator

@fruffy fruffy Jan 30, 2026

Choose a reason for hiding this comment

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

You could probably convert sourcefile to a path too, should make this easier. If we are already modernizing we could also clean up the toP4 usage of files.

Copy link
Author

Choose a reason for hiding this comment

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

Done! Added a commit using std::filesystem::relative() for the include path handling in toP4.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks. Let's convert sourceFile to std::filesystem::path also, then we should be done.

@juice928

This comment was marked as spam.

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.

4 participants