-
Notifications
You must be signed in to change notification settings - Fork 169
Adding new config options for ClangSharp #187
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
Conversation
Initial changes for win32metadata support
…is a FunctionType.
| if (type is FunctionType functionType) | ||
| { | ||
| callConv = functionType.CallConv; | ||
| if (callConv == CXCallingConv.CXCallingConv_Invalid) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Simplify the nesting level here
| if (type is FunctionType functionType) | |
| { | |
| callConv = functionType.CallConv; | |
| if (callConv == CXCallingConv.CXCallingConv_Invalid) | |
| if ((type is FunctionType functionType) && (callConv == CXCallingConv.CXCallingConv_Invalid)) |
|
|
||
| void VisitAnonymousRecordDeclFields(RecordDecl rootRecordDecl, RecordDecl anonymousRecordDecl, string contextType, string contextName) | ||
| { | ||
| if (_config.ExcludeAnonymousFieldHelpers) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think this should be a general ExcludeHelpers or that there should be an Exclude*Helper per "helper type" (just to cover any future helper additions)
| else if (underlyingType is TagType underlyingTagType) | ||
| { | ||
| // Nothing to do for tag types | ||
| // See if there's a potential typedef remapping we want to log |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Just for consistency, I think this logic should be in a ForTagType local function. It can then early exit with if (!_config.LogPotentialTypedefRemappings)
| if (_config.LogPotentialTypedefRemappings) | ||
| { | ||
| var typedefName = typedefDecl.UnderlyingDecl.Name; | ||
| var possibleNamesToRemap = new string[] { "_" + typedefName, "_tag" + typedefName, "tag" + typedefName }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just looking at the Windows bindings I've done for my own project, what about $"{typedefName}_tag", $"{typedefName}_", and $"{typedefName}__"?
| _diagnostics.Add(diagnostic); | ||
| } | ||
|
|
||
| private void AddCppAttributes(ParmVarDecl parmVarDecl, string prefix = null, string postfix = null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just want to confirm. This basically saves all the C++ attributes separated by ^ in a C# style attribute?
| return false; | ||
| } | ||
|
|
||
| if (_config.ExcludeFunctionsWithBody && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IsExcluded is actually one of the "hottest" functions in ClangSharp. It would probably be good to keep this after the IsExcludedByFile check which catches most of the exclusions.
|
Changes LGTM overall. Just a couple questions/suggestions and I think this can be merged. |
|
Going to take this and will take care of the few nits in a follow up PR. |
Some changes required for a project where we use ClangSharp to against Win32 headers so that they can be projected into other languages.