Skip to content
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

feat!: Align path_view::render_* APIs with P1036R6 #140

Merged
merged 2 commits into from
Aug 29, 2024

Conversation

BurningEnlightenment
Copy link
Collaborator

Resolves #139

Copy link

github-actions bot commented Aug 27, 2024

Unit Test Results

  6 files  ±0  192 suites  ±0   30m 39s ⏱️ + 3m 41s
 51 tests ±0   51 ✅ ±0  0 💤 ±0  0 ❌ ±0 
324 runs  ±0  324 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit bc090c4. ± Comparison against base commit e7b0ca8.

♻️ This comment has been updated with latest results.

@BurningEnlightenment
Copy link
Collaborator Author

@ned14 can you investigate the CI failures?

  • Programs / Programs (windows-2019): Tries to compile quickcpplib with valgrind support which obviously doesn't work.
  • Installability / Installability (macos-latest, *: Seems to hang during outcome installation.

@@ -1568,21 +1568,19 @@ class LLFIO_DECL path_view_component
LLFIO_TREQUIRES(LLFIO_TPRED(is_source_acceptable<T>),
Copy link

Choose a reason for hiding this comment

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

render(), immediately above this, was removed in P1036R6. Consistency dictates that either it should go away or also remove the first argument.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed, I missed the history entries

  • R5: render() function added as per LEWG request
  • R6: render() function removed as per LEWG request

🙃

However, given that render() has existed for almost two years, I would first deprecate it for one "release" and then remove it. That should ease the transition for our user base considerably at a very low cost.

Copy link
Owner

Choose a reason for hiding this comment

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

I think the number of users of those functions is likely very low, as they were only added by LEWG and as you correctly point out, were clearly added wrong by me likely very late at night when my eyes weren't working.

P1030R7 is nearing completion after a month of working on it, so I'm going to go ahead and merge this. Thanks for the changeset.

@ned14
Copy link
Owner

ned14 commented Aug 28, 2024

Programs should now be fixed on Windows. Sorry about that.

The macos installability tests take a very long time. I actually axed a bunch of stuff to make them go much faster. Even then, they are hideously slow. I assume it's an x64 box emulating a Mac M1 CPU.

I do note github actions prints a bunch of warnings about the actions being used being too old, and therefore it will only run on them node X or whatever. Which could mean "emulated slow as mollasses" but I haven't investigated.

@ned14 ned14 merged commit 2ff292f into develop Aug 29, 2024
21 checks passed
@ned14 ned14 deleted the dev/139_render_this branch August 29, 2024 10:38
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.

render functions do not match P1030R6
3 participants