Pass 1 of SilkTouch Scraper - Joint Infrastructure and XML Generation by Perksey · Pull Request #559 · dotnet/Silk.NET

Perksey · 2021-08-05T17:33:44Z

Very very wip

In-scope for this PR

Barebones joint Roslyn/CLI infrastructure is working
- Documented with for-contributors markdown
XML generation is working
- Documented with for-contributors markdown

Out-of-scope for this PR

XML parsing is working
- Documented with for-contributors markdown
XML mods are working
- Documented with for-contributors markdown
XML transformation to C# code is working
- Documented with for-contributors markdown
User documentation?

HurricanKai · 2021-08-05T17:45:54Z

@@ -0,0 +1,6 @@
+<Project>
+ <ItemGroup>
+ <AdditionalFiles Include="$(MSBuildThisFileDirectory)silktouch.json" />


That seems weird, why do all bindings share this file?

Changing, dunno why.

Changed to use one file per project with an optional "global file" for common config i.e. license header.

HurricanKai · 2021-08-05T17:46:47Z

+ "projects": {
+ "Silk.NET.Vulkan": {
+ "scraper": {
+
+ }
+ }
+ }


have you considered, instead of doing this, doing the configuration in the actual project? like... this seems pointless.

Changing to be per-project

Changed to use one file per project with an optional "global file" for common config i.e. license header.

HurricanKai · 2021-08-05T17:49:14Z

+
+namespace Silk.NET.SilkTouch.Configuration
+{
+ public static class Config


please no
Microsoft.Extensions.Configuration > static class Config

I don't think that package is applicable here. This is just a simple JSON configuration, we don't have massive infrastructure, extensibility concerns, or anything else that could possibly warrant a dedicated configuration package. I'm all for using smart libraries, but only when we actually need to do smart things. I'm not going to force packages into the codebase where they're not needed.
If you read the code for this particular class, these are just helpers and wrappers for loading configuration to reduce verbosity elsewhere in the codebase. It holds no state whatsoever.

The Configuration package is tiny and would allow us to use different configuration sources, use configuration sections, etc.
It would allow us to separate the configuration of the different stages entirely, which I think is very desirable.

The Configuration package in particular doesn't really make much sense outside of the hosting infrastructure or any other scenario where we might abstractions or injection. Moreover, it makes it far less obvious what's actually going on during configuration loading.
We really don't need this at all.

No change deemed necessary. If this is a requirement please close this PR.

HurricanKai · 2021-08-05T17:50:36Z

+ // the performance is horrible but it's not too much a cause for concern as it is a "fixed cost".
+ // definitely improve it in the future though as we do have access to new string(char*), meaning we can
+ // come up with a Span-based solution it'll just be very very very verbose.
+ if (kvp[0].ToLower().Trim() == "hint" && Enum.TryParse


Equals + IgnoreCase?

Changed to Equals + IgnoreCase.

HurricanKai · 2021-08-05T17:56:03Z

+{
+ public static class ScraperGenerationExtensions
+ {
+ public static async ValueTask<bool> RunScraperAsync<T>(this SilkTouchGenerator generator)


This doesn't feel very ValueTask to me?

This has been changed to Task.

HurricanKai · 2021-08-05T18:00:43Z

+ public sealed record SilkTouchContext
+ (
+ string AssemblyName,
+ IEnumerable<CSharpSyntaxTree> SyntaxTrees,


I need a compilation. Without a Compilation Overloads & Emitter probably won't work

What specifically do you need from the compilation though?

like the semantic model and stuff?

Ok fair enough.

Now passing the Compilation to SilkTouchContext.

Perksey · 2021-08-10T18:43:15Z

This is as far as this PR will progress. It is 100% XML documented and has contributor documentation describing the general implementation design. This is ready for review.

Perksey

Apparently I had review comments pending so forgive me if these are outdated and invalid, I can't be bothered to read through them lol.

Perksey · 2021-08-07T16:27:51Z

@@ -0,0 +1,6 @@
+<Project>
+ <ItemGroup>
+ <AdditionalFiles Include="$(MSBuildThisFileDirectory)silktouch.json" />


Changed to use one file per project with an optional "global file" for common config i.e. license header.

Perksey · 2021-08-07T16:28:45Z

+ "projects": {
+ "Silk.NET.Vulkan": {
+ "scraper": {
+
+ }
+ }
+ }


Changed to use one file per project with an optional "global file" for common config i.e. license header.

Perksey · 2021-08-07T16:29:25Z

+
+namespace Silk.NET.SilkTouch.Configuration
+{
+ public static class Config


No change deemed necessary. If this is a requirement please close this PR.

Perksey · 2021-08-07T16:30:28Z

+ // the performance is horrible but it's not too much a cause for concern as it is a "fixed cost".
+ // definitely improve it in the future though as we do have access to new string(char*), meaning we can
+ // come up with a Span-based solution it'll just be very very very verbose.
+ if (kvp[0].ToLower().Trim() == "hint" && Enum.TryParse


Changed to Equals + IgnoreCase.

Perksey · 2021-08-07T16:30:54Z

+ public sealed record SilkTouchContext
+ (
+ string AssemblyName,
+ IEnumerable<CSharpSyntaxTree> SyntaxTrees,


Now passing the Compilation to SilkTouchContext.

Perksey · 2021-08-07T16:31:08Z

+{
+ public static class ScraperGenerationExtensions
+ {
+ public static async ValueTask<bool> RunScraperAsync<T>(this SilkTouchGenerator generator)


This has been changed to Task.

Perksey · 2021-08-27T14:52:06Z

This is ready for another review.

Perksey · 2021-11-05T11:42:46Z

TODO: the Visual Studio resolution logic was backported to 2.X in Build improvements & dynamic Windows SDK resolution (backport from 3.0) #661 but further modified for stability. These changes should be brought back forward to 3.0!

HurricanKai

Merging to continue editing

…#559)

Perksey added 5 commits August 1, 2021 00:16

Starting work on SilkTouch, no Scraper-specific work yet
7edcc35

Start work on the subagent
117d4a2

Spent far too long working on a Windows SDK/MSVS resolver and not tested
4e26e3d

Scraper (up to XML gen) nearly ready&joint CLI/roslyn iface nearly ready
8e07317

Fine Kai
cac8089

Perksey added area-SilkTouch tenet-Maintainability labels Aug 5, 2021

Perksey added this to the 3.0 milestone Aug 5, 2021

Perksey self-assigned this Aug 5, 2021

Perksey changed the title ~~SilkTouch Scraper~~Pass 1 of SilkTouch Scraper - Joint Infrastructure and XML GenerationAug 5, 2021

HurricanKai reviewed Aug 5, 2021
View reviewed changes

Perksey added 5 commits August 7, 2021 17:34

Lots of changes, some review fixes, very almost generating XML now.
f40802d

More fixes
b0e80ab

XML is being generated, ClangSharp bugs to fix uncovered.
8b20fbf

Fully XML documented, markdown pending
4dbdff5

Contributor documentation
7b22e99

Perksey mentioned this pull request Aug 10, 2021
Document how to use 3.0's SilkTouch #570
Open
13 tasks

Perksey marked this pull request as ready for review August 10, 2021 18:43

Perksey commented Aug 10, 2021
View reviewed changes

Perksey requested a review from HurricanKai August 10, 2021 18:54

Perksey assigned HurricanKai and unassigned Perksey Aug 23, 2021

Perksey mentioned this pull request Oct 24, 2021
Build improvements & dynamic Windows SDK resolution (backport from 3.0) #661
Merged

HurricanKai approved these changes Jan 28, 2022
View reviewed changes

HurricanKai merged commit 3268e1b into develop/3.0Jan 28, 2022

HurricanKai deleted the feature/3.0/silktouch-scraper branch January 28, 2022 17:46

Perksey added a commit that referenced this pull request Feb 20, 2022
Pass 1 of SilkTouch Scraper - Joint Infrastructure and XML Generation (…
255ede5
…#559)

HurricanKai mentioned this pull request Feb 12, 2023
Getting back on track #1288
Closed

Uh oh!

Uh oh!

Conversation

Perksey commented Aug 5, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

In-scope for this PR

Out-of-scope for this PR

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PerkseyAug 6, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Perksey commented Aug 10, 2021

Uh oh!

Perksey left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Perksey commented Aug 27, 2021

Uh oh!

Perksey commented Nov 5, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Perksey commented Aug 5, 2021•
edited
Loading

PerkseyAug 6, 2021•
edited
Loading

Perksey left a comment •
edited
Loading

Perksey commented Nov 5, 2021•
edited
Loading