Introduce EnumDesc, add ability to output source location attribute #227
Conversation
| } | ||
| } | ||
|
|
||
| private void WriteSourceLocation(CXSourceLocation location, bool inline) |
There was a problem hiding this comment.
I'm concerned this is going to end up very noisy across regenerations. The anonymous struct names/remappings is already a pain point for some users
There was a problem hiding this comment.
It's opt-in. It is useful for diagnostics (aka "where did this come from?"), but not so much for end-user of the bindings. I don't expect SharpGen-generated bindings or e.g. TerraFX.Interop to contain this info. But e.g. win32metadata and SharpGen might use this information to enrich logging or other forms of diagnostics.
There was a problem hiding this comment.
Right, I understand the benefit.
I was just iterating the concern around noise in practice. Many diffs would become nearly unreadable due to minor changes in information such as whitespace addition.
If it's being added here, a corresponding switch and basic test validating it works and doesn't regress should also be added.
There was a problem hiding this comment.
Sure. I'll add public API surface and write some tests this weekend.
There was a problem hiding this comment.
Done. I've added 2 tests that cover all important cases.
|
The rest of the changes look reasonable to me. |
|
|
||
| GenerateTemplateBindings = 0x01000000, | ||
|
|
||
| GenerateSourceLocationAttribute = 1 << 25, |
There was a problem hiding this comment.
nit: This should be either 0x02000000 or all entries should be updated to use the 1 << n syntax.
I don't have a particular preference for which is done, I'd just like for it to be consistent so correctness can be determined at a glance
There was a problem hiding this comment.
I'll update the rest to use bit shifting to be explicit and simplify visual validation of flag values.
|
Changes LGTM, minus the differing flag value |
2 participants
Not currently exposed, but can easily be plumbed as opt-in command line option. Note, that not all Desc items have Location attribute set, since for some it doesn't make any sense.